From 298342ada6a7124905d509d7e122b67bde4e3d97 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Thu, 22 Feb 2024 16:21:49 +0200 Subject: [PATCH 01/25] Add ownership transfers for domains --- pallets/domains/src/lib.rs | 142 +++++++++++++++++++++++++++++++++++-- 1 file changed, 136 insertions(+), 6 deletions(-) diff --git a/pallets/domains/src/lib.rs b/pallets/domains/src/lib.rs index 000e6926..51755811 100644 --- a/pallets/domains/src/lib.rs +++ b/pallets/domains/src/lib.rs @@ -44,7 +44,7 @@ pub mod pallet { use sp_runtime::traits::{Saturating, StaticLookup, Zero}; use sp_std::{cmp::Ordering, convert::TryInto, vec::Vec}; - use subsocial_support::ensure_content_is_valid; + use subsocial_support::{ensure_content_is_valid, remove_from_bounded_vec}; #[pallet::config] pub trait Config: frame_system::Config + pallet_timestamp::Config { @@ -172,6 +172,10 @@ pub mod pallet { pub(super) type PaymentBeneficiary = StorageValue<_, T::AccountId, ValueQuery, DefaultPaymentBeneficiary>; + #[pallet::storage] + pub type PendingOwnershipTransfers = + StorageMap<_, Blake2_128Concat, DomainName, T::AccountId>; + #[pallet::event] #[pallet::generate_deposit(pub (super) fn deposit_event)] pub enum Event { @@ -183,6 +187,19 @@ pub mod pallet { NewWordsReserved { count: u32 }, /// Added support for new TLDs (top-level domains). NewTldsSupported { count: u32 }, + DomainOwnershipTransferCreated { + current_owner: T::AccountId, + domain: DomainName, + new_owner: T::AccountId, + }, + DomainOwnershipTransferAccepted { + account: T::AccountId, + domain: DomainName, + }, + DomainOwnershipTransferRejected { + account: T::AccountId, + domain: DomainName, + }, } #[pallet::error] @@ -226,6 +243,11 @@ pub mod pallet { InsufficientBalanceToReserveDeposit, /// There are insufficient funds to pay for the domain and reserve the deposit on it. InsufficientBalanceToRegisterDomain, + CannotTransferToCurrentOwner, + AlreadyDomainOwner, + NoPendingTransferOnDomain, + NotAllowedToAcceptOwnershipTransfer, + NotAllowedToRejectOwnershipTransfer, } #[pallet::call] @@ -426,6 +448,109 @@ pub mod pallet { Ok(Pays::No.into()) } + + #[pallet::call_index(9)] + #[pallet::weight( + T::DbWeight::get().reads_writes(1, 1) + + Weight::from_parts(36_000_000, 0) + )] + pub fn transfer_domain_ownership( + origin: OriginFor, + domain: DomainName, + transfer_to: T::AccountId, + ) -> DispatchResult { + let who = ensure_signed(origin)?; + + ensure!(who != transfer_to, Error::::CannotTransferToCurrentOwner); + + let meta = Self::require_domain(domain.clone())?; + Self::ensure_allowed_to_update_domain(&meta, &who)?; + + let domain_lc = Self::lower_domain_then_bound(&domain); + PendingOwnershipTransfers::::insert(&domain_lc, transfer_to.clone()); + + Self::deposit_event(Event::DomainOwnershipTransferCreated { + current_owner: who, + domain: domain_lc, + new_owner: transfer_to, + }); + Ok(()) + } + + #[pallet::call_index(10)] + #[pallet::weight( + T::DbWeight::get().reads_writes(1, 1) + + Weight::from_parts(60_000_000, 0) + )] + pub fn accept_pending_ownership_transfer( + origin: OriginFor, + domain: DomainName, + ) -> DispatchResult { + let new_owner = ensure_signed(origin)?; + + let meta = Self::require_domain(domain.clone())?; + ensure!(meta.owner != new_owner, Error::::AlreadyDomainOwner); + + let domain_lc = Self::lower_domain_then_bound(&domain); + let transfer_to = + PendingOwnershipTransfers::::get(&domain_lc).ok_or(Error::::NoPendingTransferOnDomain)?; + + ensure!(new_owner == transfer_to, Error::::NotAllowedToAcceptOwnershipTransfer); + + Self::ensure_domains_limit_not_reached(&transfer_to)?; + Self::ensure_can_pay_for_domain(&new_owner, Zero::zero(), meta.domain_deposit.deposit)?; + + // Here we know that the origin is eligible to become a new owner of this domain. + PendingOwnershipTransfers::::remove(&domain_lc); + + DomainsByOwner::::mutate(&meta.owner, |domains| { + remove_from_bounded_vec(domains, domain_lc.clone()) + }); + + DomainsByOwner::::mutate(&new_owner, |domains| { + domains.try_push(domain_lc.clone()).expect("qed; too many domains per account") + }); + + RegisteredDomains::::mutate(&domain_lc, |stored_meta_opt| { + if let Some(stored_meta) = stored_meta_opt { + stored_meta.owner = new_owner.clone(); + } + }); + + Self::deposit_event(Event::DomainOwnershipTransferAccepted { + account: new_owner, + domain: domain_lc, + }); + Ok(()) + } + + #[pallet::call_index(11)] + #[pallet::weight( + T::DbWeight::get().reads_writes(1, 1) + + Weight::from_parts(40_000_000, 0) + )] + pub fn reject_pending_ownership_transfer( + origin: OriginFor, + domain: DomainName, + ) -> DispatchResult { + let who = ensure_signed(origin)?; + + let meta = Self::require_domain(domain.clone())?; + + let domain_lc = Self::lower_domain_then_bound(&domain); + let transfer_to = + PendingOwnershipTransfers::::get(&domain_lc).ok_or(Error::::NoPendingTransferOnDomain)?; + + ensure!( + who == transfer_to || who == meta.owner, + Error::::NotAllowedToRejectOwnershipTransfer + ); + + PendingOwnershipTransfers::::remove(&domain_lc); + + Self::deposit_event(Event::DomainOwnershipTransferRejected { account: who, domain: domain_lc }); + Ok(()) + } } impl Pallet { @@ -468,11 +593,7 @@ pub mod pallet { Error::::DomainAlreadyOwned, ); - let recipient_domains_count = Self::domains_by_owner(&recipient).len(); - ensure!( - recipient_domains_count < T::MaxDomainsPerAccount::get() as usize, - Error::::TooManyDomainsPerAccount, - ); + Self::ensure_domains_limit_not_reached(&recipient)?; let deposit_info: DomainDeposit> = (caller.clone(), T::BaseDomainDeposit::get()).into(); @@ -717,5 +838,14 @@ pub mod pallet { &owner, price, WithdrawReasons::TRANSFER, owner_balance.saturating_sub(total_price) ) } + + fn ensure_domains_limit_not_reached(account: &T::AccountId) -> DispatchResult { + let domains_count = Self::domains_by_owner(account).len(); + ensure!( + domains_count < T::MaxDomainsPerAccount::get() as usize, + Error::::TooManyDomainsPerAccount, + ); + Ok(()) + } } } From 389f38ddec866b6998fc5ebb999f90f1bd3733f1 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Fri, 23 Feb 2024 12:55:27 +0200 Subject: [PATCH 02/25] Rename pallet-space-ownership to pallet-ownership --- integration-tests/Cargo.toml | 4 ++-- integration-tests/src/mock.rs | 4 ++-- integration-tests/src/tests/space_ownership.rs | 2 +- pallets/posts/tests/Cargo.toml | 4 ++-- pallets/posts/tests/src/mock.rs | 4 ++-- pallets/space-ownership/Cargo.toml | 4 ++-- pallets/space-ownership/src/weights.rs | 8 ++++---- pallets/space-ownership/tests/Cargo.toml | 4 ++-- pallets/space-ownership/tests/src/mock.rs | 4 ++-- pallets/space-ownership/tests/src/tests.rs | 2 +- runtime/Cargo.toml | 8 ++++---- runtime/src/lib.rs | 8 ++++---- 12 files changed, 28 insertions(+), 28 deletions(-) diff --git a/integration-tests/Cargo.toml b/integration-tests/Cargo.toml index 13b8c91b..c19ee5dd 100644 --- a/integration-tests/Cargo.toml +++ b/integration-tests/Cargo.toml @@ -28,7 +28,7 @@ std = [ 'pallet-reactions/std', 'pallet-roles/std', 'pallet-space-follows/std', - 'pallet-space-ownership/std', + 'pallet-ownership/std', 'pallet-spaces/std', 'subsocial-support/std', ] @@ -53,7 +53,7 @@ pallet-profiles = { path = '../pallets/profiles', default-features = false } pallet-reactions = { path = '../pallets/reactions', default-features = false } pallet-roles = { path = '../pallets/roles', default-features = false } pallet-space-follows = { path = '../pallets/space-follows', default-features = false } -pallet-space-ownership = { path = '../pallets/space-ownership', default-features = false } +pallet-ownership = { path = '../pallets/space-ownership', default-features = false } pallet-spaces = { path = '../pallets/spaces', default-features = false } subsocial-support = { path = '../pallets/support', default-features = false } diff --git a/integration-tests/src/mock.rs b/integration-tests/src/mock.rs index 3bbacbb1..a5ba3398 100644 --- a/integration-tests/src/mock.rs +++ b/integration-tests/src/mock.rs @@ -48,7 +48,7 @@ frame_support::construct_runtime!( Reactions: pallet_reactions, Roles: pallet_roles, SpaceFollows: pallet_space_follows, - SpaceOwnership: pallet_space_ownership, + SpaceOwnership: pallet_ownership, Spaces: pallet_spaces, } ); @@ -160,7 +160,7 @@ impl pallet_space_follows::Config for TestRuntime { type WeightInfo = pallet_space_follows::weights::SubstrateWeight; } -impl pallet_space_ownership::Config for TestRuntime { +impl pallet_ownership::Config for TestRuntime { type RuntimeEvent = RuntimeEvent; type ProfileManager = Profiles; type CreatorStakingProvider = (); diff --git a/integration-tests/src/tests/space_ownership.rs b/integration-tests/src/tests/space_ownership.rs index d03f72ec..2f9cf51d 100644 --- a/integration-tests/src/tests/space_ownership.rs +++ b/integration-tests/src/tests/space_ownership.rs @@ -7,7 +7,7 @@ use frame_support::{assert_ok, assert_noop}; use sp_runtime::traits::Zero; -use pallet_space_ownership::Error as SpaceOwnershipError; +use pallet_ownership::Error as SpaceOwnershipError; use pallet_spaces::Error as SpacesError; use crate::mock::*; diff --git a/pallets/posts/tests/Cargo.toml b/pallets/posts/tests/Cargo.toml index 9449c692..4a94129f 100644 --- a/pallets/posts/tests/Cargo.toml +++ b/pallets/posts/tests/Cargo.toml @@ -34,7 +34,7 @@ sp-io = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0 pallet-balances = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.40", default-features = false } pallet-roles = { default-features = false, path = '../../roles' } pallet-space-follows = { default-features = false, path = '../../space-follows' } -pallet-space-ownership = { default-features = false, path = '../../space-ownership' } +pallet-ownership = { default-features = false, path = '../../space-ownership' } pallet-profiles = { default-features = false, path = '../../profiles' } pallet-posts = { default-features = false, path = '..' } pallet-spaces = { default-features = false, path = '../../spaces' } @@ -53,7 +53,7 @@ std = [ 'pallet-balances/std', 'pallet-roles/std', 'pallet-space-follows/std', - 'pallet-space-ownership/std', + 'pallet-ownership/std', 'pallet-profiles/std', 'pallet-posts/std', ] diff --git a/pallets/posts/tests/src/mock.rs b/pallets/posts/tests/src/mock.rs index 747142c1..e8fbd857 100644 --- a/pallets/posts/tests/src/mock.rs +++ b/pallets/posts/tests/src/mock.rs @@ -32,7 +32,7 @@ frame_support::construct_runtime!( SpaceFollows: pallet_space_follows, Posts: pallet_posts, Spaces: pallet_spaces, - SpaceOwnership: pallet_space_ownership, + SpaceOwnership: pallet_ownership, } ); @@ -150,7 +150,7 @@ impl pallet_space_follows::Config for Test { type WeightInfo = (); } -impl pallet_space_ownership::Config for Test { +impl pallet_ownership::Config for Test { type RuntimeEvent = RuntimeEvent; type ProfileManager = Profiles; type CreatorStakingProvider = (); diff --git a/pallets/space-ownership/Cargo.toml b/pallets/space-ownership/Cargo.toml index ccce28a6..16197678 100644 --- a/pallets/space-ownership/Cargo.toml +++ b/pallets/space-ownership/Cargo.toml @@ -1,12 +1,12 @@ [package] -name = 'pallet-space-ownership' +name = 'pallet-ownership' version = '0.2.2' authors = ['DappForce '] edition = '2021' license = 'GPL-3.0-only' homepage = 'https://subsocial.network' repository = 'https://github.com/dappforce/subsocial-parachain' -description = 'Pallet to manage space ownership: transfer, accept, reject' +description = 'Pallet to manage spaces/posts/domains ownership: transfer, accept, reject' keywords = ['blockchain', 'cryptocurrency', 'social-network', 'news-feed', 'marketplace'] categories = ['cryptography::cryptocurrencies'] diff --git a/pallets/space-ownership/src/weights.rs b/pallets/space-ownership/src/weights.rs index 05c67e33..c0eac355 100644 --- a/pallets/space-ownership/src/weights.rs +++ b/pallets/space-ownership/src/weights.rs @@ -5,7 +5,7 @@ // Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE -//! Autogenerated weights for pallet_space_ownership +//! Autogenerated weights for pallet_ownership //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev //! DATE: 2023-02-15, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]` @@ -23,7 +23,7 @@ // --wasm-execution // Compiled // --pallet - // pallet_space_ownership + // pallet_ownership // --extrinsic // * // --steps @@ -45,14 +45,14 @@ use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; use sp_std::marker::PhantomData; -/// Weight functions needed for pallet_space_ownership. +/// Weight functions needed for pallet_ownership. pub trait WeightInfo { fn transfer_space_ownership() -> Weight; fn accept_pending_ownership() -> Weight; fn reject_pending_ownership() -> Weight; } -/// Weights for pallet_space_ownership using the Substrate node and recommended hardware. +/// Weights for pallet_ownership using the Substrate node and recommended hardware. pub struct SubstrateWeight(PhantomData); impl WeightInfo for SubstrateWeight { // Storage: Spaces SpaceById (r:1 w:0) diff --git a/pallets/space-ownership/tests/Cargo.toml b/pallets/space-ownership/tests/Cargo.toml index ecc7b4b5..8e201206 100644 --- a/pallets/space-ownership/tests/Cargo.toml +++ b/pallets/space-ownership/tests/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = 'pallet-space-ownership-tests' +name = 'pallet-ownership-tests' version = '0.2.2' authors = ['DappForce '] edition = '2021' @@ -21,7 +21,7 @@ scale-info = { version = "2.2.0", default-features = false, features = ["derive" # Local dependencies subsocial-support = { default-features = false, path = '../../support' } pallet-permissions = { default-features = false, path = '../../permissions' } -pallet-space-ownership = { default-features = false, path = '..' } +pallet-ownership = { default-features = false, path = '..' } # Substrate dependencies pallet-timestamp = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } diff --git a/pallets/space-ownership/tests/src/mock.rs b/pallets/space-ownership/tests/src/mock.rs index 7b7c544d..7410d60a 100644 --- a/pallets/space-ownership/tests/src/mock.rs +++ b/pallets/space-ownership/tests/src/mock.rs @@ -28,7 +28,7 @@ frame_support::construct_runtime!( Roles: pallet_roles, Profiles: pallet_profiles, SpaceFollows: pallet_space_follows, - SpaceOwnership: pallet_space_ownership, + SpaceOwnership: pallet_ownership, Spaces: pallet_spaces, } ); @@ -136,7 +136,7 @@ impl pallet_space_follows::Config for Test { type WeightInfo = (); } -impl pallet_space_ownership::Config for Test { +impl pallet_ownership::Config for Test { type RuntimeEvent = RuntimeEvent; type ProfileManager = Profiles; type CreatorStakingProvider = (); diff --git a/pallets/space-ownership/tests/src/tests.rs b/pallets/space-ownership/tests/src/tests.rs index 4ae0cece..55897343 100644 --- a/pallets/space-ownership/tests/src/tests.rs +++ b/pallets/space-ownership/tests/src/tests.rs @@ -7,7 +7,7 @@ use frame_support::{assert_noop, assert_ok}; use sp_runtime::traits::Zero; -use pallet_space_ownership::Error as SpaceOwnershipError; +use pallet_ownership::Error as SpaceOwnershipError; use pallet_spaces::Error as SpacesError; use crate::{mock::*, tests_utils::*}; diff --git a/runtime/Cargo.toml b/runtime/Cargo.toml index 7351f10b..c534f7c5 100644 --- a/runtime/Cargo.toml +++ b/runtime/Cargo.toml @@ -36,7 +36,7 @@ pallet-reactions = { path = '../pallets/reactions', default-features = false } pallet-resource-discussions = { path = '../pallets/resource-discussions', default-features = false } pallet-roles = { path = '../pallets/roles', default-features = false } pallet-space-follows = { path = '../pallets/space-follows', default-features = false } -pallet-space-ownership = { path = '../pallets/space-ownership', default-features = false } +pallet-ownership = { path = '../pallets/space-ownership', default-features = false } pallet-spaces = { path = '../pallets/spaces', default-features = false } subsocial-support = { path = '../pallets/support', default-features = false } pallet-free-proxy = { path = "../pallets/free-proxy", default-features = false } @@ -171,7 +171,7 @@ std = [ "pallet-resource-discussions/std", "pallet-roles/std", "pallet-space-follows/std", - "pallet-space-ownership/std", + "pallet-ownership/std", "pallet-spaces/std", "subsocial-support/std", "pallet-free-proxy/std", @@ -208,7 +208,7 @@ runtime-benchmarks = [ "pallet-resource-discussions/runtime-benchmarks", "pallet-roles/runtime-benchmarks", "pallet-space-follows/runtime-benchmarks", - "pallet-space-ownership/runtime-benchmarks", + "pallet-ownership/runtime-benchmarks", "pallet-spaces/runtime-benchmarks", "pallet-post-follows/runtime-benchmarks", "pallet-posts/runtime-benchmarks", @@ -255,7 +255,7 @@ try-runtime = [ "pallet-resource-discussions/try-runtime", "pallet-roles/try-runtime", "pallet-space-follows/try-runtime", - "pallet-space-ownership/try-runtime", + "pallet-ownership/try-runtime", "pallet-spaces/try-runtime", "pallet-evm-addresses/try-runtime", ] diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index c8c849b8..601194db 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -770,11 +770,11 @@ impl pallet_spaces::Config for Runtime { type WeightInfo = pallet_spaces::weights::SubstrateWeight; } -impl pallet_space_ownership::Config for Runtime { +impl pallet_ownership::Config for Runtime { type RuntimeEvent = RuntimeEvent; type ProfileManager = Profiles; type CreatorStakingProvider = CreatorStaking; - type WeightInfo = pallet_space_ownership::weights::SubstrateWeight; + type WeightInfo = pallet_ownership::weights::SubstrateWeight; } impl pallet_account_follows::Config for Runtime { @@ -906,7 +906,7 @@ construct_runtime!( AccountFollows: pallet_account_follows = 72, Profiles: pallet_profiles = 73, SpaceFollows: pallet_space_follows = 74, - SpaceOwnership: pallet_space_ownership = 75, + SpaceOwnership: pallet_ownership = 75, Spaces: pallet_spaces = 76, PostFollows: pallet_post_follows = 77, Posts: pallet_posts = 78, @@ -937,7 +937,7 @@ mod benches { [pallet_reactions, Reactions] [pallet_roles, Roles] [pallet_space_follows, SpaceFollows] - [pallet_space_ownership, SpaceOwnership] + [pallet_ownership, SpaceOwnership] [pallet_spaces, Spaces] [pallet_post_follows, PostFollows] [pallet_posts, Posts] From cdfb88310e0d30f00289b24e5f63e674af820b35 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Fri, 23 Feb 2024 16:09:41 +0200 Subject: [PATCH 03/25] WIP: refactor ownership pallet to work with different entities --- .../src/tests/space_ownership.rs | 6 ++-- .../src/utils/space_ownership_utils.rs | 2 +- pallets/posts/tests/src/tests_utils.rs | 2 +- pallets/space-ownership/Cargo.toml | 4 +-- pallets/space-ownership/src/benchmarking.rs | 4 +-- pallets/space-ownership/src/lib.rs | 31 +++++++++---------- pallets/space-ownership/tests/src/tests.rs | 6 ++-- .../space-ownership/tests/src/tests_utils.rs | 2 +- 8 files changed, 28 insertions(+), 29 deletions(-) diff --git a/integration-tests/src/tests/space_ownership.rs b/integration-tests/src/tests/space_ownership.rs index 2f9cf51d..f9efe09d 100644 --- a/integration-tests/src/tests/space_ownership.rs +++ b/integration-tests/src/tests/space_ownership.rs @@ -96,7 +96,7 @@ fn accept_pending_ownership_should_fail_when_no_pending_transfer_for_space() { ExtBuilder::build_with_space().execute_with(|| { assert_noop!( _accept_default_pending_ownership(), - SpaceOwnershipError::::NoPendingTransferOnSpace + SpaceOwnershipError::::NoPendingTransfer ); }); } @@ -108,7 +108,7 @@ fn accept_pending_ownership_should_fail_if_origin_is_already_an_owner() { assert_noop!( _accept_pending_ownership(Some(RuntimeOrigin::signed(ACCOUNT1)), None), - SpaceOwnershipError::::AlreadyASpaceOwner + SpaceOwnershipError::::AlreadyOwner ); }); } @@ -172,7 +172,7 @@ fn reject_pending_ownership_should_fail_when_no_pending_transfer_on_space() { ExtBuilder::build_with_space().execute_with(|| { assert_noop!( _reject_default_pending_ownership(), - SpaceOwnershipError::::NoPendingTransferOnSpace + SpaceOwnershipError::::NoPendingTransfer ); // Rejecting a transfer from ACCOUNT2 }); } diff --git a/integration-tests/src/utils/space_ownership_utils.rs b/integration-tests/src/utils/space_ownership_utils.rs index 7552b9f6..de5119fc 100644 --- a/integration-tests/src/utils/space_ownership_utils.rs +++ b/integration-tests/src/utils/space_ownership_utils.rs @@ -20,7 +20,7 @@ pub(crate) fn _transfer_space_ownership( space_id: Option, transfer_to: Option, ) -> DispatchResult { - SpaceOwnership::transfer_space_ownership( + SpaceOwnership::transfer_ownership( origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT1)), space_id.unwrap_or(SPACE1), transfer_to.unwrap_or(ACCOUNT2), diff --git a/pallets/posts/tests/src/tests_utils.rs b/pallets/posts/tests/src/tests_utils.rs index b71d2a55..77b7e5f6 100644 --- a/pallets/posts/tests/src/tests_utils.rs +++ b/pallets/posts/tests/src/tests_utils.rs @@ -486,7 +486,7 @@ pub(crate) fn _transfer_space_ownership( space_id: Option, transfer_to: Option, ) -> DispatchResult { - SpaceOwnership::transfer_space_ownership( + SpaceOwnership::transfer_ownership( origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT1)), space_id.unwrap_or(SPACE1), transfer_to.unwrap_or(ACCOUNT2), diff --git a/pallets/space-ownership/Cargo.toml b/pallets/space-ownership/Cargo.toml index 16197678..93844746 100644 --- a/pallets/space-ownership/Cargo.toml +++ b/pallets/space-ownership/Cargo.toml @@ -20,7 +20,7 @@ std = [ 'frame-support/std', 'frame-system/std', 'sp-std/std', - 'pallet-spaces/std', + 'pallet-permissions/std', 'subsocial-support/std', ] try-runtime = ['frame-support/try-runtime'] @@ -30,7 +30,7 @@ codec = { package = "parity-scale-codec", version = "3.0.0", default-features = scale-info = { version = "2.2.0", default-features = false, features = ["derive"] } # Local dependencies -pallet-spaces = { default-features = false, path = '../spaces' } +pallet-permissions = { default-features = false, path = '../permissions' } subsocial-support = { default-features = false, path = '../support' } # Substrate dependencies diff --git a/pallets/space-ownership/src/benchmarking.rs b/pallets/space-ownership/src/benchmarking.rs index 6d6223c5..f8ad5991 100644 --- a/pallets/space-ownership/src/benchmarking.rs +++ b/pallets/space-ownership/src/benchmarking.rs @@ -45,7 +45,7 @@ benchmarks! { let acc2 = account::("Acc2", 2, 0); let space = create_dummy_space::(RawOrigin::Signed(acc1.clone()))?; - Pallet::::transfer_space_ownership( + Pallet::::transfer_ownership( RawOrigin::Signed(acc1.clone()).into(), space.id, acc2.clone(), @@ -64,7 +64,7 @@ benchmarks! { let acc2 = account::("Acc2", 2, 0); let space = create_dummy_space::(RawOrigin::Signed(acc1.clone()))?; - Pallet::::transfer_space_ownership( + Pallet::::transfer_ownership( RawOrigin::Signed(acc1.clone()).into(), space.id, acc2.clone(), diff --git a/pallets/space-ownership/src/lib.rs b/pallets/space-ownership/src/lib.rs index 20c8d6b4..ed7156db 100644 --- a/pallets/space-ownership/src/lib.rs +++ b/pallets/space-ownership/src/lib.rs @@ -9,7 +9,6 @@ use frame_system::ensure_signed; use sp_std::prelude::*; -use pallet_spaces::{Pallet as Spaces, SpaceById, SpaceIdsByOwner}; use subsocial_support::{ remove_from_bounded_vec, traits::IsAccountBlocked, ModerationError, SpaceId, }; @@ -45,14 +44,14 @@ pub mod pallet { #[pallet::error] pub enum Error { - /// The current space owner cannot transfer ownership to themself. + /// The current entity owner cannot transfer ownership to themselves. CannotTransferToCurrentOwner, - /// Account is already an owner of a space. - AlreadyASpaceOwner, + /// Account is already an owner of an entity. + AlreadyOwner, /// Cannot transfer ownership, because a space is registered as an active creator. ActiveCreatorCannotTransferOwnership, - /// There is no pending ownership transfer for a given space. - NoPendingTransferOnSpace, + /// There is no pending ownership transfer for a given entity. + NoPendingTransfer, /// Account is not allowed to accept ownership transfer. NotAllowedToAcceptOwnershipTransfer, /// Account is not allowed to reject ownership transfer. @@ -66,18 +65,18 @@ pub mod pallet { #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event { - SpaceOwnershipTransferCreated { + OwnershipTransferCreated { current_owner: T::AccountId, - space_id: SpaceId, + entity: SocialEntities, new_owner: T::AccountId, }, - SpaceOwnershipTransferAccepted { + OwnershipTransferAccepted { account: T::AccountId, - space_id: SpaceId, + entity: SocialEntities, }, - SpaceOwnershipTransferRejected { + OwnershipTransferRejected { account: T::AccountId, - space_id: SpaceId, + entity: SocialEntities, }, } @@ -85,9 +84,9 @@ pub mod pallet { impl Pallet { #[pallet::call_index(0)] #[pallet::weight(::WeightInfo::transfer_space_ownership())] - pub fn transfer_space_ownership( + pub fn transfer_ownership( origin: OriginFor, - space_id: SpaceId, + entity: SocialEntities, transfer_to: T::AccountId, ) -> DispatchResult { let who = ensure_signed(origin)?; @@ -105,9 +104,9 @@ pub mod pallet { PendingSpaceOwner::::insert(space_id, transfer_to.clone()); - Self::deposit_event(Event::SpaceOwnershipTransferCreated { + Self::deposit_event(Event::OwnershipTransferCreated { current_owner: who, - space_id, + entity, new_owner: transfer_to, }); Ok(()) diff --git a/pallets/space-ownership/tests/src/tests.rs b/pallets/space-ownership/tests/src/tests.rs index 55897343..5cec2043 100644 --- a/pallets/space-ownership/tests/src/tests.rs +++ b/pallets/space-ownership/tests/src/tests.rs @@ -82,7 +82,7 @@ fn accept_pending_ownership_should_fail_when_no_pending_transfer_for_space() { ExtBuilder::build_with_space().execute_with(|| { assert_noop!( _accept_default_pending_ownership(), - SpaceOwnershipError::::NoPendingTransferOnSpace + SpaceOwnershipError::::NoPendingTransfer ); }); } @@ -94,7 +94,7 @@ fn accept_pending_ownership_should_fail_if_origin_is_already_an_owner() { assert_noop!( _accept_pending_ownership(Some(RuntimeOrigin::signed(ACCOUNT1)), None), - SpaceOwnershipError::::AlreadyASpaceOwner + SpaceOwnershipError::::AlreadyOwner ); }); } @@ -155,7 +155,7 @@ fn reject_pending_ownership_should_fail_when_no_pending_transfer_on_space() { ExtBuilder::build_with_space().execute_with(|| { assert_noop!( _reject_default_pending_ownership(), - SpaceOwnershipError::::NoPendingTransferOnSpace + SpaceOwnershipError::::NoPendingTransfer ); // Rejecting a transfer from ACCOUNT2 }); } diff --git a/pallets/space-ownership/tests/src/tests_utils.rs b/pallets/space-ownership/tests/src/tests_utils.rs index 8b705280..5fa698c2 100644 --- a/pallets/space-ownership/tests/src/tests_utils.rs +++ b/pallets/space-ownership/tests/src/tests_utils.rs @@ -106,7 +106,7 @@ pub(crate) fn _transfer_space_ownership( space_id: Option, transfer_to: Option, ) -> DispatchResult { - SpaceOwnership::transfer_space_ownership( + SpaceOwnership::transfer_ownership( origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT1)), space_id.unwrap_or(SPACE1), transfer_to.unwrap_or(ACCOUNT2), From 9410f9412e8878b89fb50491300e558977f3fdd4 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Fri, 23 Feb 2024 16:31:01 +0200 Subject: [PATCH 04/25] Provide interfaces for changing spaces and domains ownership --- pallets/domains/src/lib.rs | 160 ++++++++------------------- pallets/spaces/src/lib.rs | 21 ++++ pallets/support/src/traits.rs | 2 +- pallets/support/src/traits/common.rs | 10 ++ 4 files changed, 80 insertions(+), 113 deletions(-) diff --git a/pallets/domains/src/lib.rs b/pallets/domains/src/lib.rs index 51755811..3446b482 100644 --- a/pallets/domains/src/lib.rs +++ b/pallets/domains/src/lib.rs @@ -44,7 +44,7 @@ pub mod pallet { use sp_runtime::traits::{Saturating, StaticLookup, Zero}; use sp_std::{cmp::Ordering, convert::TryInto, vec::Vec}; - use subsocial_support::{ensure_content_is_valid, remove_from_bounded_vec}; + use subsocial_support::{ensure_content_is_valid, remove_from_bounded_vec, traits::DomainsProvider}; #[pallet::config] pub trait Config: frame_system::Config + pallet_timestamp::Config { @@ -210,9 +210,10 @@ pub mod pallet { TooManyDomainsPerAccount, /// This domain label may contain only a-z, 0-9 and hyphen characters. DomainContainsInvalidChar, - /// This domain name length must be within the limits defined by - /// [`Config::MinDomainLength`] and [`Config::MaxDomainLength`] characters, inclusive. + /// This domain name length must be greater or equal the [`Config::MinDomainLength`] limit. DomainIsTooShort, + /// This domain name length must be below or equal the [`Config::MaxDomainLength`] limit. + DomainIsTooLong, /// This domain has expired. DomainHasExpired, /// Domain was not found by the domain name. @@ -243,11 +244,6 @@ pub mod pallet { InsufficientBalanceToReserveDeposit, /// There are insufficient funds to pay for the domain and reserve the deposit on it. InsufficientBalanceToRegisterDomain, - CannotTransferToCurrentOwner, - AlreadyDomainOwner, - NoPendingTransferOnDomain, - NotAllowedToAcceptOwnershipTransfer, - NotAllowedToRejectOwnershipTransfer, } #[pallet::call] @@ -448,109 +444,6 @@ pub mod pallet { Ok(Pays::No.into()) } - - #[pallet::call_index(9)] - #[pallet::weight( - T::DbWeight::get().reads_writes(1, 1) + - Weight::from_parts(36_000_000, 0) - )] - pub fn transfer_domain_ownership( - origin: OriginFor, - domain: DomainName, - transfer_to: T::AccountId, - ) -> DispatchResult { - let who = ensure_signed(origin)?; - - ensure!(who != transfer_to, Error::::CannotTransferToCurrentOwner); - - let meta = Self::require_domain(domain.clone())?; - Self::ensure_allowed_to_update_domain(&meta, &who)?; - - let domain_lc = Self::lower_domain_then_bound(&domain); - PendingOwnershipTransfers::::insert(&domain_lc, transfer_to.clone()); - - Self::deposit_event(Event::DomainOwnershipTransferCreated { - current_owner: who, - domain: domain_lc, - new_owner: transfer_to, - }); - Ok(()) - } - - #[pallet::call_index(10)] - #[pallet::weight( - T::DbWeight::get().reads_writes(1, 1) + - Weight::from_parts(60_000_000, 0) - )] - pub fn accept_pending_ownership_transfer( - origin: OriginFor, - domain: DomainName, - ) -> DispatchResult { - let new_owner = ensure_signed(origin)?; - - let meta = Self::require_domain(domain.clone())?; - ensure!(meta.owner != new_owner, Error::::AlreadyDomainOwner); - - let domain_lc = Self::lower_domain_then_bound(&domain); - let transfer_to = - PendingOwnershipTransfers::::get(&domain_lc).ok_or(Error::::NoPendingTransferOnDomain)?; - - ensure!(new_owner == transfer_to, Error::::NotAllowedToAcceptOwnershipTransfer); - - Self::ensure_domains_limit_not_reached(&transfer_to)?; - Self::ensure_can_pay_for_domain(&new_owner, Zero::zero(), meta.domain_deposit.deposit)?; - - // Here we know that the origin is eligible to become a new owner of this domain. - PendingOwnershipTransfers::::remove(&domain_lc); - - DomainsByOwner::::mutate(&meta.owner, |domains| { - remove_from_bounded_vec(domains, domain_lc.clone()) - }); - - DomainsByOwner::::mutate(&new_owner, |domains| { - domains.try_push(domain_lc.clone()).expect("qed; too many domains per account") - }); - - RegisteredDomains::::mutate(&domain_lc, |stored_meta_opt| { - if let Some(stored_meta) = stored_meta_opt { - stored_meta.owner = new_owner.clone(); - } - }); - - Self::deposit_event(Event::DomainOwnershipTransferAccepted { - account: new_owner, - domain: domain_lc, - }); - Ok(()) - } - - #[pallet::call_index(11)] - #[pallet::weight( - T::DbWeight::get().reads_writes(1, 1) + - Weight::from_parts(40_000_000, 0) - )] - pub fn reject_pending_ownership_transfer( - origin: OriginFor, - domain: DomainName, - ) -> DispatchResult { - let who = ensure_signed(origin)?; - - let meta = Self::require_domain(domain.clone())?; - - let domain_lc = Self::lower_domain_then_bound(&domain); - let transfer_to = - PendingOwnershipTransfers::::get(&domain_lc).ok_or(Error::::NoPendingTransferOnDomain)?; - - ensure!( - who == transfer_to || who == meta.owner, - Error::::NotAllowedToRejectOwnershipTransfer - ); - - PendingOwnershipTransfers::::remove(&domain_lc); - - Self::deposit_event(Event::DomainOwnershipTransferRejected { account: who, domain: domain_lc }); - Ok(()) - } } impl Pallet { @@ -758,11 +651,20 @@ pub mod pallet { Ok(domains.len() as u32) } - /// Try to get domain meta by it's custom and top-level domain names. + /// Try to get domain meta by its custom and top-level domain names. pub fn require_domain(domain: DomainName) -> Result, DispatchError> { Ok(Self::registered_domain(domain).ok_or(Error::::DomainNotFound)?) } + /// Try to get domain meta by its custom and top-level domain names. + /// This function pre-validates the passed argument. + pub fn require_domain_from_ref(domain: &[u8]) -> Result, DispatchError> { + ensure!(domain.len() <= T::MaxDomainLength::get() as usize, Error::::DomainIsTooLong); + let domain_lc = Self::lower_domain_then_bound(domain); + + Self::require_domain(domain_lc) + } + /// Check that the domain is not expired and [sender] is the current owner. pub fn ensure_allowed_to_update_domain( domain_meta: &DomainMeta, @@ -848,4 +750,38 @@ pub mod pallet { Ok(()) } } + + impl DomainsProvider for Pallet { + fn ensure_allowed_to_update_domain(account: &T::AccountId, domain: &[u8]) -> DispatchResult { + let meta = Self::require_domain_from_ref(domain)?; + Self::ensure_allowed_to_update_domain(&meta, account) + } + + fn change_domain_owner(domain: &[u8], new_owner: &T::AccountId) -> DispatchResult { + let meta = Self::require_domain_from_ref(domain)?; + let domain_lc = Self::lower_domain_then_bound(domain); + + Self::ensure_domains_limit_not_reached(&new_owner)?; + Self::ensure_can_pay_for_domain(&new_owner, Zero::zero(), meta.domain_deposit.deposit)?; + + // Here we know that the origin is eligible to become a new owner of this domain. + PendingOwnershipTransfers::::remove(&domain_lc); + + DomainsByOwner::::mutate(&meta.owner, |domains| { + remove_from_bounded_vec(domains, domain_lc.clone()) + }); + + DomainsByOwner::::mutate(&new_owner, |domains| { + domains.try_push(domain_lc.clone()).expect("qed; too many domains per account") + }); + + RegisteredDomains::::mutate(&domain_lc, |stored_meta_opt| { + if let Some(stored_meta) = stored_meta_opt { + stored_meta.owner = new_owner.clone(); + } + }); + + Ok(()) + } + } } diff --git a/pallets/spaces/src/lib.rs b/pallets/spaces/src/lib.rs index a10d4408..6ec4b24a 100644 --- a/pallets/spaces/src/lib.rs +++ b/pallets/spaces/src/lib.rs @@ -431,6 +431,27 @@ pub mod pallet { let space = Pallet::::require_space(space_id)?; Ok(space.owner) } + + fn change_space_owner(space_id: SpaceId, new_owner: T::AccountId) -> DispatchResult { + Self::ensure_space_limit_not_reached(&new_owner)?; + let space = Pallet::::require_space(space_id)?; + + // TODO: reuse copy-pasted parts of code + SpaceIdsByOwner::::mutate(&space.owner, |ids| { + remove_from_bounded_vec(ids, space_id) + }); + + SpaceIdsByOwner::::mutate(&new_owner, |ids| { + ids.try_push(space_id).expect("qed; too many spaces per account") + }); + + SpaceById::::mutate(space_id, |stored_space_opt| { + if let Some(stored_space) = stored_space_opt { + stored_space.owner = new_owner; + } + }); + Ok(()) + } fn create_space(owner: &T::AccountId, content: Content) -> Result { Self::do_create_space(owner, content, None) diff --git a/pallets/support/src/traits.rs b/pallets/support/src/traits.rs index 28955eb9..9727800d 100644 --- a/pallets/support/src/traits.rs +++ b/pallets/support/src/traits.rs @@ -5,7 +5,7 @@ // Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE pub use common::{ - CreatorStakingProvider, ProfileManager, SpaceFollowsProvider, SpacePermissionsProvider, + CreatorStakingProvider, DomainsProvider, ProfileManager, SpaceFollowsProvider, SpacePermissionsProvider, SpacesInterface, PostFollowsProvider, }; pub use moderation::{IsAccountBlocked, IsContentBlocked, IsPostBlocked, IsSpaceBlocked}; diff --git a/pallets/support/src/traits/common.rs b/pallets/support/src/traits/common.rs index 9db5805a..4f1fe7e3 100644 --- a/pallets/support/src/traits/common.rs +++ b/pallets/support/src/traits/common.rs @@ -32,8 +32,12 @@ pub trait ProfileManager { fn unlink_space_from_profile(account: &AccountId, space_id: SpaceId); } +// TODO: rename to `SpacesProvider` pub trait SpacesInterface { + fn get_space_owner(space_id: SpaceId) -> Result; + + fn change_space_owner(space_id: SpaceId, new_owner: AccountId) -> DispatchResult; fn create_space(owner: &AccountId, content: Content) -> Result; } @@ -51,3 +55,9 @@ impl CreatorStakingProvider for () { false } } + +pub trait DomainsProvider { + fn ensure_allowed_to_update_domain(account: &AccountId, domain: &[u8]) -> DispatchResult; + + fn change_domain_owner(domain: &[u8], new_owner: &AccountId) -> DispatchResult; +} From fc76af72c68bba5cd399dce92842d7c4d21e3556 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Tue, 27 Feb 2024 12:47:29 +0200 Subject: [PATCH 05/25] Introduce PostsProvider - Refactor other providers traits --- pallets/domains/src/lib.rs | 23 ++++++++++++++++------- pallets/posts/src/functions.rs | 24 ++++++++++++++++++++++++ pallets/spaces/src/lib.rs | 2 +- pallets/support/src/traits.rs | 4 ++-- pallets/support/src/traits/common.rs | 16 ++++++++++++++-- 5 files changed, 57 insertions(+), 12 deletions(-) diff --git a/pallets/domains/src/lib.rs b/pallets/domains/src/lib.rs index 3446b482..a10ee577 100644 --- a/pallets/domains/src/lib.rs +++ b/pallets/domains/src/lib.rs @@ -750,22 +750,31 @@ pub mod pallet { Ok(()) } } - + impl DomainsProvider for Pallet { + type DomainLength = T::MaxDomainLength; + + fn get_domain_owner(domain: &[u8]) -> Result { + let meta = Self::require_domain_from_ref(domain)?; + Ok(meta.owner) + } + fn ensure_allowed_to_update_domain(account: &T::AccountId, domain: &[u8]) -> DispatchResult { let meta = Self::require_domain_from_ref(domain)?; Self::ensure_allowed_to_update_domain(&meta, account) } - fn change_domain_owner(domain: &[u8], new_owner: &T::AccountId) -> DispatchResult { + fn update_domain_owner(domain: &[u8], new_owner: &T::AccountId) -> DispatchResult { let meta = Self::require_domain_from_ref(domain)?; let domain_lc = Self::lower_domain_then_bound(domain); - + Self::ensure_domains_limit_not_reached(&new_owner)?; Self::ensure_can_pay_for_domain(&new_owner, Zero::zero(), meta.domain_deposit.deposit)?; - - // Here we know that the origin is eligible to become a new owner of this domain. - PendingOwnershipTransfers::::remove(&domain_lc); + + if !meta.domain_deposit.deposit.is_zero() { + T::Currency::reserve(&new_owner, meta.domain_deposit.deposit)?; + T::Currency::unreserve(&meta.domain_deposit.depositor, meta.domain_deposit.deposit); + } DomainsByOwner::::mutate(&meta.owner, |domains| { remove_from_bounded_vec(domains, domain_lc.clone()) @@ -780,7 +789,7 @@ pub mod pallet { stored_meta.owner = new_owner.clone(); } }); - + Ok(()) } } diff --git a/pallets/posts/src/functions.rs b/pallets/posts/src/functions.rs index 1670a340..54b544a7 100644 --- a/pallets/posts/src/functions.rs +++ b/pallets/posts/src/functions.rs @@ -8,6 +8,7 @@ use frame_support::dispatch::DispatchResult; use sp_runtime::traits::Saturating; use subsocial_support::{remove_from_vec, SpaceId}; +use subsocial_support::traits::PostsProvider; use super::*; @@ -453,3 +454,26 @@ impl Pallet { Ok(()) } } + +impl PostsProvider for Pallet { + fn get_post_owner(post_id: PostId) -> Result { + let post = Self::require_post(post_id)?; + Ok(post.owner) + } + + fn ensure_allowed_to_update_post(account: &T::AccountId, post_id: PostId) -> DispatchResult { + let post = Self::require_post(post_id)?; + let space = post.get_space()?; + Self::ensure_account_can_update_post(account, &post, &space) + } + + fn update_post_owner(post_id: PostId, new_owner: &T::AccountId) -> DispatchResult { + PostById::::mutate(post_id, |stored_post_opt| { + if let Some(stored_post) = stored_post_opt { + stored_post.owner = new_owner.clone(); + } + }); + + Ok(()) + } +} diff --git a/pallets/spaces/src/lib.rs b/pallets/spaces/src/lib.rs index 6ec4b24a..ba1a1228 100644 --- a/pallets/spaces/src/lib.rs +++ b/pallets/spaces/src/lib.rs @@ -432,7 +432,7 @@ pub mod pallet { Ok(space.owner) } - fn change_space_owner(space_id: SpaceId, new_owner: T::AccountId) -> DispatchResult { + fn update_space_owner(space_id: SpaceId, new_owner: T::AccountId) -> DispatchResult { Self::ensure_space_limit_not_reached(&new_owner)?; let space = Pallet::::require_space(space_id)?; diff --git a/pallets/support/src/traits.rs b/pallets/support/src/traits.rs index 9727800d..fd033b5b 100644 --- a/pallets/support/src/traits.rs +++ b/pallets/support/src/traits.rs @@ -5,8 +5,8 @@ // Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE pub use common::{ - CreatorStakingProvider, DomainsProvider, ProfileManager, SpaceFollowsProvider, SpacePermissionsProvider, - SpacesInterface, PostFollowsProvider, + CreatorStakingProvider, DomainsProvider, PostFollowsProvider, PostsProvider, ProfileManager, + SpaceFollowsProvider, SpacePermissionsProvider, SpacesInterface, }; pub use moderation::{IsAccountBlocked, IsContentBlocked, IsPostBlocked, IsSpaceBlocked}; diff --git a/pallets/support/src/traits/common.rs b/pallets/support/src/traits/common.rs index 4f1fe7e3..5a4b46fa 100644 --- a/pallets/support/src/traits/common.rs +++ b/pallets/support/src/traits/common.rs @@ -37,7 +37,7 @@ pub trait SpacesInterface { fn get_space_owner(space_id: SpaceId) -> Result; - fn change_space_owner(space_id: SpaceId, new_owner: AccountId) -> DispatchResult; + fn update_space_owner(space_id: SpaceId, new_owner: AccountId) -> DispatchResult; fn create_space(owner: &AccountId, content: Content) -> Result; } @@ -57,7 +57,19 @@ impl CreatorStakingProvider for () { } pub trait DomainsProvider { + type DomainLength: frame_support::traits::Get; + + fn get_domain_owner(domain: &[u8]) -> Result; + fn ensure_allowed_to_update_domain(account: &AccountId, domain: &[u8]) -> DispatchResult; - fn change_domain_owner(domain: &[u8], new_owner: &AccountId) -> DispatchResult; + fn update_domain_owner(domain: &[u8], new_owner: &AccountId) -> DispatchResult; +} + +pub trait PostsProvider { + fn get_post_owner(post_id: PostId) -> Result; + + fn ensure_allowed_to_update_post(account: &AccountId, post_id: PostId) -> DispatchResult; + + fn update_post_owner(post_id: PostId, new_owner: &AccountId) -> DispatchResult; } From 172b49a46c47df50f23c81aa221a4edf540941e8 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Tue, 27 Feb 2024 12:48:17 +0200 Subject: [PATCH 06/25] Remove redundant code from domains pallet --- pallets/domains/src/lib.rs | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/pallets/domains/src/lib.rs b/pallets/domains/src/lib.rs index a10ee577..f2410a2d 100644 --- a/pallets/domains/src/lib.rs +++ b/pallets/domains/src/lib.rs @@ -172,10 +172,6 @@ pub mod pallet { pub(super) type PaymentBeneficiary = StorageValue<_, T::AccountId, ValueQuery, DefaultPaymentBeneficiary>; - #[pallet::storage] - pub type PendingOwnershipTransfers = - StorageMap<_, Blake2_128Concat, DomainName, T::AccountId>; - #[pallet::event] #[pallet::generate_deposit(pub (super) fn deposit_event)] pub enum Event { @@ -187,19 +183,6 @@ pub mod pallet { NewWordsReserved { count: u32 }, /// Added support for new TLDs (top-level domains). NewTldsSupported { count: u32 }, - DomainOwnershipTransferCreated { - current_owner: T::AccountId, - domain: DomainName, - new_owner: T::AccountId, - }, - DomainOwnershipTransferAccepted { - account: T::AccountId, - domain: DomainName, - }, - DomainOwnershipTransferRejected { - account: T::AccountId, - domain: DomainName, - }, } #[pallet::error] From d206aacd0858aa98befdcd53a7832807723a71b3 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Tue, 27 Feb 2024 12:49:20 +0200 Subject: [PATCH 07/25] Implement transfer ownership logic in ownership pallet --- pallets/space-ownership/Cargo.toml | 2 + pallets/space-ownership/src/lib.rs | 141 +++++++++++++++++------------ 2 files changed, 85 insertions(+), 58 deletions(-) diff --git a/pallets/space-ownership/Cargo.toml b/pallets/space-ownership/Cargo.toml index 93844746..64e890d6 100644 --- a/pallets/space-ownership/Cargo.toml +++ b/pallets/space-ownership/Cargo.toml @@ -19,6 +19,7 @@ std = [ 'frame-benchmarking/std', 'frame-support/std', 'frame-system/std', + 'sp-runtime/std', 'sp-std/std', 'pallet-permissions/std', 'subsocial-support/std', @@ -37,4 +38,5 @@ subsocial-support = { default-features = false, path = '../support' } frame-benchmarking = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false, optional = true } frame-support = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } frame-system = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } +sp-runtime = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } sp-std = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } diff --git a/pallets/space-ownership/src/lib.rs b/pallets/space-ownership/src/lib.rs index ed7156db..49ba8181 100644 --- a/pallets/space-ownership/src/lib.rs +++ b/pallets/space-ownership/src/lib.rs @@ -9,10 +9,6 @@ use frame_system::ensure_signed; use sp_std::prelude::*; -use subsocial_support::{ - remove_from_bounded_vec, traits::IsAccountBlocked, ModerationError, SpaceId, -}; - pub use pallet::*; #[cfg(feature = "runtime-benchmarks")] @@ -25,17 +21,40 @@ pub mod pallet { use crate::weights::WeightInfo; use frame_support::pallet_prelude::*; use frame_system::pallet_prelude::*; - - use subsocial_support::traits::{CreatorStakingProvider, ProfileManager}; + use pallet_permissions::SpacePermissions; + + use subsocial_support::{PostId, SpaceId, SpacePermissionsInfo, traits::{CreatorStakingProvider, DomainsProvider, ProfileManager, SpacesInterface, PostsProvider, SpacePermissionsProvider}}; + + pub(crate) type DomainLengthOf = + <::DomainsProvider as DomainsProvider<::AccountId>>::DomainLength; + + #[derive(Encode, Decode, Clone, Eq, PartialEq, RuntimeDebugNoBound, TypeInfo, MaxEncodedLen)] + #[scale_info(skip_type_params(T))] + pub enum EntityWithOwnership { + Space(SpaceId), + Post(PostId), + Domain(BoundedVec>), + } + + pub(crate) type SpacePermissionsInfoOf = + SpacePermissionsInfo<::AccountId, SpacePermissions>; #[pallet::config] - pub trait Config: frame_system::Config + pallet_spaces::Config { + pub trait Config: frame_system::Config + pallet_permissions::Config { type RuntimeEvent: From> + IsType<::RuntimeEvent>; type ProfileManager: ProfileManager; + type SpacesInterface: SpacesInterface; + + type SpacePermissionsProvider: SpacePermissionsProvider>; + type CreatorStakingProvider: CreatorStakingProvider; + type DomainsProvider: DomainsProvider; + + type PostsProvider: PostsProvider; + type WeightInfo: WeightInfo; } @@ -59,24 +78,25 @@ pub mod pallet { } #[pallet::storage] - #[pallet::getter(fn pending_space_owner)] - pub type PendingSpaceOwner = StorageMap<_, Twox64Concat, SpaceId, T::AccountId>; + #[pallet::getter(fn pending_ownership_transfer)] + pub type PendingOwnershipTransfers = + StorageMap<_, Twox64Concat, EntityWithOwnership, T::AccountId>; #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event { OwnershipTransferCreated { current_owner: T::AccountId, - entity: SocialEntities, + entity: EntityWithOwnership, new_owner: T::AccountId, }, OwnershipTransferAccepted { account: T::AccountId, - entity: SocialEntities, + entity: EntityWithOwnership, }, OwnershipTransferRejected { account: T::AccountId, - entity: SocialEntities, + entity: EntityWithOwnership, }, } @@ -86,23 +106,31 @@ pub mod pallet { #[pallet::weight(::WeightInfo::transfer_space_ownership())] pub fn transfer_ownership( origin: OriginFor, - entity: SocialEntities, + entity: EntityWithOwnership, transfer_to: T::AccountId, ) -> DispatchResult { let who = ensure_signed(origin)?; - let space = Spaces::::require_space(space_id)?; - space.ensure_space_owner(who.clone())?; + match entity.clone() { + EntityWithOwnership::Space(space_id) => { + ensure!(who != transfer_to, Error::::CannotTransferToCurrentOwner); - ensure!(who != transfer_to, Error::::CannotTransferToCurrentOwner); - ensure!( - T::IsAccountBlocked::is_allowed_account(transfer_to.clone(), space_id), - ModerationError::AccountIsBlocked - ); + T::SpacePermissionsProvider::ensure_space_owner(space_id, &who)?; + // TODO: ensure we need this kind of moderation checks + // ensure!( + // T::IsAccountBlocked::is_allowed_account(transfer_to.clone(), space_id), + // ModerationError::AccountIsBlocked + // ); - Self::ensure_not_active_creator(space_id)?; + Self::ensure_not_active_creator(space_id)?; + } + EntityWithOwnership::Post(post_id) => + T::PostsProvider::ensure_allowed_to_update_post(&who, post_id)?, + EntityWithOwnership::Domain(domain) => + T::DomainsProvider::ensure_allowed_to_update_domain(&who, &domain)?, + } - PendingSpaceOwner::::insert(space_id, transfer_to.clone()); + PendingOwnershipTransfers::::insert(&entity, transfer_to.clone()); Self::deposit_event(Event::OwnershipTransferCreated { current_owner: who, @@ -114,66 +142,54 @@ pub mod pallet { #[pallet::call_index(1)] #[pallet::weight(::WeightInfo::accept_pending_ownership())] - pub fn accept_pending_ownership(origin: OriginFor, space_id: SpaceId) -> DispatchResult { + pub fn accept_pending_ownership(origin: OriginFor, entity: EntityWithOwnership) -> DispatchResult { let new_owner = ensure_signed(origin)?; - let mut space = Spaces::require_space(space_id)?; - ensure!(!space.is_owner(&new_owner), Error::::AlreadyASpaceOwner); - let transfer_to = - Self::pending_space_owner(space_id).ok_or(Error::::NoPendingTransferOnSpace)?; + Self::pending_ownership_transfer(&entity).ok_or(Error::::NoPendingTransfer)?; ensure!(new_owner == transfer_to, Error::::NotAllowedToAcceptOwnershipTransfer); - Self::ensure_not_active_creator(space_id)?; - - Spaces::::ensure_space_limit_not_reached(&transfer_to)?; - - // Here we know that the origin is eligible to become a new owner of this space. - PendingSpaceOwner::::remove(space_id); + match entity.clone() { + EntityWithOwnership::Space(space_id) => { + let old_space_owner = Self::get_entity_owner(&entity)?; - let old_owner = space.owner; - space.owner = new_owner.clone(); - SpaceById::::insert(space_id, space); + Self::ensure_not_active_creator(space_id)?; + T::SpacesInterface::update_space_owner(space_id, transfer_to.clone())?; + T::ProfileManager::unlink_space_from_profile(&old_space_owner, space_id); + } + EntityWithOwnership::Post(post_id) => + T::PostsProvider::update_post_owner(post_id, &new_owner)?, + EntityWithOwnership::Domain(domain) => + T::DomainsProvider::update_domain_owner(&domain, &new_owner)?, + } - T::ProfileManager::unlink_space_from_profile(&old_owner, space_id); + PendingOwnershipTransfers::::remove(&entity); - // Remove space id from the list of spaces by old owner - SpaceIdsByOwner::::mutate(old_owner, |space_ids| { - remove_from_bounded_vec(space_ids, space_id) - }); - - // Add space id to the list of spaces by new owner - SpaceIdsByOwner::::mutate(new_owner.clone(), |ids| { - ids.try_push(space_id).expect("qed; too many spaces per account") - }); - - // TODO add a new owner as a space follower? See - // T::BeforeSpaceCreated::before_space_created(new_owner.clone(), space)?; - - Self::deposit_event(Event::SpaceOwnershipTransferAccepted { + Self::deposit_event(Event::OwnershipTransferAccepted { account: new_owner, - space_id, + entity, }); Ok(()) } #[pallet::call_index(2)] #[pallet::weight(::WeightInfo::reject_pending_ownership())] - pub fn reject_pending_ownership(origin: OriginFor, space_id: SpaceId) -> DispatchResult { + pub fn reject_pending_ownership(origin: OriginFor, entity: EntityWithOwnership) -> DispatchResult { let who = ensure_signed(origin)?; - let space = Spaces::::require_space(space_id)?; let transfer_to = - Self::pending_space_owner(space_id).ok_or(Error::::NoPendingTransferOnSpace)?; + Self::pending_ownership_transfer(&entity).ok_or(Error::::NoPendingTransfer)?; + let entity_owner = Self::get_entity_owner(&entity)?; + ensure!( - who == transfer_to || who == space.owner, + who == transfer_to || who == entity_owner, Error::::NotAllowedToRejectOwnershipTransfer ); - PendingSpaceOwner::::remove(space_id); + PendingOwnershipTransfers::::remove(&entity); - Self::deposit_event(Event::SpaceOwnershipTransferRejected { account: who, space_id }); + Self::deposit_event(Event::OwnershipTransferRejected { account: who, entity }); Ok(()) } } @@ -184,7 +200,16 @@ pub mod pallet { !T::CreatorStakingProvider::is_creator_active(creator_id), Error::::ActiveCreatorCannotTransferOwnership, ); + Ok(()) } + + fn get_entity_owner(entity: &EntityWithOwnership) -> Result { + match entity { + EntityWithOwnership::Space(space_id) => T::SpacesInterface::get_space_owner(*space_id), + EntityWithOwnership::Post(post_id) => T::PostsProvider::get_post_owner(*post_id), + EntityWithOwnership::Domain(domain) => T::DomainsProvider::get_domain_owner(domain), + } + } } } From 4f1cafbc318e60d75cb67e4da15b4e50a57e3028 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Tue, 27 Feb 2024 12:51:59 +0200 Subject: [PATCH 08/25] Add migration to the ownership pallet Fix runtime --- pallets/space-ownership/src/lib.rs | 5 ++ pallets/space-ownership/src/migration.rs | 95 ++++++++++++++++++++++++ runtime/src/lib.rs | 9 ++- 3 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 pallets/space-ownership/src/migration.rs diff --git a/pallets/space-ownership/src/lib.rs b/pallets/space-ownership/src/lib.rs index 49ba8181..434ce341 100644 --- a/pallets/space-ownership/src/lib.rs +++ b/pallets/space-ownership/src/lib.rs @@ -14,6 +14,7 @@ pub use pallet::*; #[cfg(feature = "runtime-benchmarks")] mod benchmarking; pub mod weights; +pub mod migration; #[frame_support::pallet] pub mod pallet { @@ -58,7 +59,11 @@ pub mod pallet { type WeightInfo: WeightInfo; } + /// The current storage version + const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + #[pallet::pallet] + #[pallet::storage_version(STORAGE_VERSION)] pub struct Pallet(PhantomData); #[pallet::error] diff --git a/pallets/space-ownership/src/migration.rs b/pallets/space-ownership/src/migration.rs new file mode 100644 index 00000000..4e17c938 --- /dev/null +++ b/pallets/space-ownership/src/migration.rs @@ -0,0 +1,95 @@ +// Copyright (C) DAPPFORCE PTE. LTD. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0. +// +// Full notice is available at https://github.com/dappforce/subsocial-parachain/blob/main/COPYRIGHT +// Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE + +use frame_support::{log, traits::OnRuntimeUpgrade}; +use sp_runtime::{Saturating, traits::Zero}; +#[cfg(feature = "try-runtime")] +use sp_std::vec::Vec; + +use super::*; + +const LOG_TARGET: &'static str = "runtime::ownership"; + +pub mod v1 { + use frame_support::{pallet_prelude::*, storage_alias, weights::Weight}; + use subsocial_support::SpaceId; + + use super::*; + + #[storage_alias] + pub type PendingSpaceOwner = + StorageMap, Twox64Concat, SpaceId, ::AccountId>; + + pub struct MigrateToV1(sp_std::marker::PhantomData); + + impl OnRuntimeUpgrade for MigrateToV1 { + fn on_runtime_upgrade() -> Weight { + let current_version = Pallet::::current_storage_version(); + let onchain_version = Pallet::::on_chain_storage_version(); + + log::info!( + target: LOG_TARGET, + "Running migration with current storage version {:?} / onchain {:?}", + current_version, + onchain_version + ); + + if onchain_version == 0 && current_version == 1 { + let mut migrated = 0u64; + + for (space_id, account) in PendingSpaceOwner::::drain() { + PendingOwnershipTransfers::::insert(EntityWithOwnership::Space(space_id), account); + migrated.saturating_inc(); + } + + current_version.put::>(); + + log::info!( + target: LOG_TARGET, + "Upgraded {} records, storage to version {:?}", + migrated, + current_version + ); + T::DbWeight::get().reads_writes(migrated + 1, migrated + 1) + } else { + log::info!( + target: LOG_TARGET, + "Migration did not execute. This probably should be removed" + ); + T::DbWeight::get().reads(1) + } + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, &'static str> { + let current_version = Pallet::::current_storage_version(); + let onchain_version = Pallet::::on_chain_storage_version(); + ensure!(onchain_version == 0 && current_version == 1, "migration from version 0 to 1."); + let prev_count = PendingSpaceOwner::::iter().count(); + Ok((prev_count as u32).encode()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(prev_count: Vec) -> Result<(), &'static str> { + let prev_count: u32 = Decode::decode(&mut prev_count.as_slice()).expect( + "the state parameter should be something that was generated by pre_upgrade", + ); + let post_count = PendingOwnershipTransfers::::iter().count() as u32; + let old_storage_count = PendingSpaceOwner::::iter().count(); + + ensure!( + prev_count == post_count, + "the records count before and after the migration should be the same" + ); + ensure!(old_storage_count.is_zero(), "all records should be migrated"); + + ensure!(Pallet::::on_chain_storage_version() == 1, "wrong storage version"); + + Ok(()) + } + } +} + diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 601194db..6f6d2864 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -121,7 +121,10 @@ pub type Executive = frame_executive::Executive< frame_system::ChainContext, Runtime, AllPalletsWithSystem, - pallet_creator_staking::migration::MigrateToV1, + ( + pallet_creator_staking::migration::MigrateToV1, + pallet_ownership::migration::v1::MigrateToV1, + ), >; /// Handles converting a weight scalar to a fee value, based on the scale and granularity of the @@ -773,7 +776,11 @@ impl pallet_spaces::Config for Runtime { impl pallet_ownership::Config for Runtime { type RuntimeEvent = RuntimeEvent; type ProfileManager = Profiles; + type SpacesInterface = Spaces; + type SpacePermissionsProvider = Spaces; type CreatorStakingProvider = CreatorStaking; + type DomainsProvider = Domains; + type PostsProvider = Posts; type WeightInfo = pallet_ownership::weights::SubstrateWeight; } From ab8559fa7a7aada59cf08cac0081282f01993e31 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Tue, 27 Feb 2024 12:52:50 +0200 Subject: [PATCH 09/25] Fix mocks --- Cargo.lock | 85 ++++++++++++----------- pallets/space-ownership/tests/Cargo.toml | 6 +- pallets/space-ownership/tests/src/mock.rs | 39 +++++++++++ 3 files changed, 87 insertions(+), 43 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7c4006e1..9e199ddc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3557,13 +3557,13 @@ dependencies = [ "frame-support", "frame-system", "pallet-balances", + "pallet-ownership", "pallet-permissions", "pallet-posts", "pallet-profiles", "pallet-reactions", "pallet-roles", "pallet-space-follows", - "pallet-space-ownership", "pallet-spaces", "pallet-timestamp", "parity-scale-codec", @@ -6076,6 +6076,47 @@ dependencies = [ "sp-std", ] +[[package]] +name = "pallet-ownership" +version = "0.2.2" +dependencies = [ + "frame-benchmarking", + "frame-support", + "frame-system", + "pallet-permissions", + "parity-scale-codec", + "scale-info", + "sp-runtime", + "sp-std", + "subsocial-support", +] + +[[package]] +name = "pallet-ownership-tests" +version = "0.2.2" +dependencies = [ + "frame-support", + "frame-system", + "impl-trait-for-tuples", + "pallet-balances", + "pallet-domains", + "pallet-ownership", + "pallet-permissions", + "pallet-posts", + "pallet-profiles", + "pallet-roles", + "pallet-space-follows", + "pallet-spaces", + "pallet-timestamp", + "parity-scale-codec", + "scale-info", + "sp-core", + "sp-io", + "sp-runtime", + "sp-std", + "subsocial-support", +] + [[package]] name = "pallet-permissions" version = "0.2.2" @@ -6156,12 +6197,12 @@ dependencies = [ "frame-support", "frame-system", "pallet-balances", + "pallet-ownership", "pallet-permissions", "pallet-posts", "pallet-profiles", "pallet-roles", "pallet-space-follows", - "pallet-space-ownership", "pallet-spaces", "pallet-timestamp", "parity-scale-codec", @@ -6468,44 +6509,6 @@ dependencies = [ "subsocial-support", ] -[[package]] -name = "pallet-space-ownership" -version = "0.2.2" -dependencies = [ - "frame-benchmarking", - "frame-support", - "frame-system", - "pallet-spaces", - "parity-scale-codec", - "scale-info", - "sp-std", - "subsocial-support", -] - -[[package]] -name = "pallet-space-ownership-tests" -version = "0.2.2" -dependencies = [ - "frame-support", - "frame-system", - "impl-trait-for-tuples", - "pallet-balances", - "pallet-permissions", - "pallet-profiles", - "pallet-roles", - "pallet-space-follows", - "pallet-space-ownership", - "pallet-spaces", - "pallet-timestamp", - "parity-scale-codec", - "scale-info", - "sp-core", - "sp-io", - "sp-runtime", - "sp-std", - "subsocial-support", -] - [[package]] name = "pallet-spaces" version = "0.2.2" @@ -11755,6 +11758,7 @@ dependencies = [ "pallet-free-proxy", "pallet-insecure-randomness-collective-flip", "pallet-multisig", + "pallet-ownership", "pallet-permissions", "pallet-post-follows", "pallet-posts", @@ -11766,7 +11770,6 @@ dependencies = [ "pallet-roles", "pallet-session", "pallet-space-follows", - "pallet-space-ownership", "pallet-spaces", "pallet-sudo", "pallet-timestamp", diff --git a/pallets/space-ownership/tests/Cargo.toml b/pallets/space-ownership/tests/Cargo.toml index 8e201206..24098b54 100644 --- a/pallets/space-ownership/tests/Cargo.toml +++ b/pallets/space-ownership/tests/Cargo.toml @@ -34,8 +34,10 @@ sp-std = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v sp-core = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } sp-io = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } pallet-balances = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.40", default-features = false } -pallet-roles = { default-features = false, path = '../../roles' } +pallet-domains = { default-features = false, path = '../../domains' } +pallet-posts = { default-features = false, path = '../../posts' } pallet-profiles = { default-features = false, path = '../../profiles' } +pallet-roles = { default-features = false, path = '../../roles' } pallet-spaces = { default-features = false, path = '../../spaces' } pallet-space-follows = { default-features = false, path = '../../space-follows' } @@ -51,8 +53,8 @@ std = [ 'sp-std/std', 'pallet-permissions/std', 'pallet-balances/std', + 'pallet-profiles/std', 'pallet-roles/std', 'pallet-space-follows/std', - 'pallet-profiles/std', ] try-runtime = ["frame-support/try-runtime"] diff --git a/pallets/space-ownership/tests/src/mock.rs b/pallets/space-ownership/tests/src/mock.rs index 7410d60a..bd648045 100644 --- a/pallets/space-ownership/tests/src/mock.rs +++ b/pallets/space-ownership/tests/src/mock.rs @@ -30,6 +30,8 @@ frame_support::construct_runtime!( SpaceFollows: pallet_space_follows, SpaceOwnership: pallet_ownership, Spaces: pallet_spaces, + Domains: pallet_domains, + Posts: pallet_posts, } ); @@ -37,6 +39,8 @@ pub(super) type AccountId = u64; pub(super) type Balance = u64; pub(super) type BlockNumber = u64; +pub(crate) const DOMAIN_DEPOSIT: Balance = 10; + parameter_types! { pub const BlockHashCount: u64 = 250; pub const SS58Prefix: u8 = 42; @@ -136,9 +140,44 @@ impl pallet_space_follows::Config for Test { type WeightInfo = (); } +impl pallet_posts::Config for Test { + type RuntimeEvent = RuntimeEvent; + type MaxCommentDepth = ConstU32<10>; + type IsPostBlocked = (); + type WeightInfo = (); +} + +parameter_types! { + pub const BaseDomainDeposit: Balance = DOMAIN_DEPOSIT; + pub const OuterValueByteDeposit: Balance = 5; + pub const RegistrationPeriodLimit: BlockNumber = 5; + pub const InitialPaymentBeneficiary: AccountId = 1; + pub InitialPricesConfig: pallet_domains::types::PricesConfigVec = Vec::new(); +} + +impl pallet_domains::Config for Test { + type RuntimeEvent = RuntimeEvent; + type Currency = Balances; + type MinDomainLength = ConstU32<1>; + type MaxDomainLength = ConstU32<64>; + type MaxDomainsPerAccount = ConstU32<5>; + type DomainsInsertLimit = ConstU32<5>; + type RegistrationPeriodLimit = RegistrationPeriodLimit; + type MaxOuterValueLength = ConstU32<64>; + type BaseDomainDeposit = BaseDomainDeposit; + type OuterValueByteDeposit = OuterValueByteDeposit; + type InitialPaymentBeneficiary = InitialPaymentBeneficiary; + type InitialPricesConfig = InitialPricesConfig; + type WeightInfo = (); +} + impl pallet_ownership::Config for Test { type RuntimeEvent = RuntimeEvent; type ProfileManager = Profiles; + type SpacesInterface = Spaces; + type SpacePermissionsProvider = Spaces; type CreatorStakingProvider = (); + type DomainsProvider = Domains; + type PostsProvider = Posts; type WeightInfo = (); } From 35993d01c648d65f3e10d29df64cc75bacf332d6 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Tue, 27 Feb 2024 14:04:09 +0200 Subject: [PATCH 10/25] Add migration to the runtime --- runtime/src/lib.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 6f6d2864..c04ff020 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -121,10 +121,7 @@ pub type Executive = frame_executive::Executive< frame_system::ChainContext, Runtime, AllPalletsWithSystem, - ( - pallet_creator_staking::migration::MigrateToV1, - pallet_ownership::migration::v1::MigrateToV1, - ), + pallet_ownership::migration::v1::MigrateToV1, >; /// Handles converting a weight scalar to a fee value, based on the scale and granularity of the From 149efe16cac07caa866d9c184f0d6ea8d6d5fea6 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Tue, 27 Feb 2024 14:04:17 +0200 Subject: [PATCH 11/25] Update spec_version to 41 --- runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index c04ff020..03007898 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -178,7 +178,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: create_runtime_str!("subsocial-parachain"), impl_name: create_runtime_str!("subsocial-parachain"), authoring_version: 1, - spec_version: 40, + spec_version: 41, impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 8, From a01fb066f85b8d5ce882de1c4304961bb2740af1 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Tue, 27 Feb 2024 14:04:31 +0200 Subject: [PATCH 12/25] Update transaction_version to 9 --- runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 03007898..173ce6ed 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -181,7 +181,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_version: 41, impl_version: 0, apis: RUNTIME_API_VERSIONS, - transaction_version: 8, + transaction_version: 9, state_version: 0, }; From beb18a6e425f357ef970d59ed6ad7575c196d802 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Tue, 27 Feb 2024 17:12:52 +0200 Subject: [PATCH 13/25] Fix code formatting in migration file --- pallets/space-ownership/src/migration.rs | 51 +++++++++++++----------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/pallets/space-ownership/src/migration.rs b/pallets/space-ownership/src/migration.rs index 4e17c938..225f6f72 100644 --- a/pallets/space-ownership/src/migration.rs +++ b/pallets/space-ownership/src/migration.rs @@ -5,7 +5,9 @@ // Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE use frame_support::{log, traits::OnRuntimeUpgrade}; -use sp_runtime::{Saturating, traits::Zero}; +use sp_runtime::Saturating; +#[cfg(feature = "try-runtime")] +use sp_runtime::traits::Zero; #[cfg(feature = "try-runtime")] use sp_std::vec::Vec; @@ -15,12 +17,13 @@ const LOG_TARGET: &'static str = "runtime::ownership"; pub mod v1 { use frame_support::{pallet_prelude::*, storage_alias, weights::Weight}; + use subsocial_support::SpaceId; use super::*; - + #[storage_alias] - pub type PendingSpaceOwner = + pub type PendingSpaceOwner = StorageMap, Twox64Concat, SpaceId, ::AccountId>; pub struct MigrateToV1(sp_std::marker::PhantomData); @@ -31,34 +34,37 @@ pub mod v1 { let onchain_version = Pallet::::on_chain_storage_version(); log::info!( - target: LOG_TARGET, - "Running migration with current storage version {:?} / onchain {:?}", - current_version, - onchain_version - ); + target: LOG_TARGET, + "Running migration with current storage version {:?} / onchain {:?}", + current_version, + onchain_version + ); if onchain_version == 0 && current_version == 1 { let mut migrated = 0u64; - + for (space_id, account) in PendingSpaceOwner::::drain() { - PendingOwnershipTransfers::::insert(EntityWithOwnership::Space(space_id), account); + PendingOwnershipTransfers::::insert( + EntityWithOwnership::Space(space_id), + account, + ); migrated.saturating_inc(); } current_version.put::>(); log::info!( - target: LOG_TARGET, - "Upgraded {} records, storage to version {:?}", - migrated, - current_version - ); + target: LOG_TARGET, + "Upgraded {} records, storage to version {:?}", + migrated, + current_version + ); T::DbWeight::get().reads_writes(migrated + 1, migrated + 1) } else { log::info!( - target: LOG_TARGET, - "Migration did not execute. This probably should be removed" - ); + target: LOG_TARGET, + "Migration did not execute. This probably should be removed" + ); T::DbWeight::get().reads(1) } } @@ -79,11 +85,11 @@ pub mod v1 { ); let post_count = PendingOwnershipTransfers::::iter().count() as u32; let old_storage_count = PendingSpaceOwner::::iter().count(); - + ensure!( - prev_count == post_count, - "the records count before and after the migration should be the same" - ); + prev_count == post_count, + "the records count before and after the migration should be the same" + ); ensure!(old_storage_count.is_zero(), "all records should be migrated"); ensure!(Pallet::::on_chain_storage_version() == 1, "wrong storage version"); @@ -92,4 +98,3 @@ pub mod v1 { } } } - From 3212eddafb808bed248a88be5803565c5e782c77 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Thu, 14 Mar 2024 15:17:32 +0200 Subject: [PATCH 14/25] chore: Add tests for ownership pallet (#256) * Add tests for ownership pallet * Add utility function to ownership for benchmarking * Update benchmarks in ownership pallet --------- Co-authored-by: Oleh Mell --- Cargo.lock | 6 + integration-tests/Cargo.toml | 2 + integration-tests/src/mock.rs | 30 ++ .../src/tests/space_ownership.rs | 12 +- .../src/utils/space_ownership_utils.rs | 7 +- pallets/creator-staking/src/tests/mock.rs | 2 + pallets/domains/Cargo.toml | 5 +- pallets/domains/src/lib.rs | 16 + pallets/posts/src/functions.rs | 13 + pallets/posts/tests/Cargo.toml | 3 + pallets/posts/tests/src/mock.rs | 39 ++- pallets/posts/tests/src/tests_utils.rs | 5 +- pallets/profiles/src/mock.rs | 2 + pallets/space-ownership/Cargo.toml | 8 +- pallets/space-ownership/src/benchmarking.rs | 157 ++++++--- pallets/space-ownership/src/lib.rs | 20 +- pallets/space-ownership/src/weights.rs | 327 +++++++++++++----- pallets/space-ownership/tests/Cargo.toml | 3 + pallets/space-ownership/tests/src/mock.rs | 42 ++- pallets/space-ownership/tests/src/tests.rs | 242 +++++++------ .../space-ownership/tests/src/tests_utils.rs | 229 ++++++++---- pallets/support/Cargo.toml | 3 + pallets/support/src/traits/common.rs | 6 + runtime/src/lib.rs | 1 + 24 files changed, 854 insertions(+), 326 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9e199ddc..729399e1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3557,6 +3557,7 @@ dependencies = [ "frame-support", "frame-system", "pallet-balances", + "pallet-domains", "pallet-ownership", "pallet-permissions", "pallet-posts", @@ -6083,6 +6084,7 @@ dependencies = [ "frame-benchmarking", "frame-support", "frame-system", + "pallet-balances", "pallet-permissions", "parity-scale-codec", "scale-info", @@ -6098,6 +6100,8 @@ dependencies = [ "frame-support", "frame-system", "impl-trait-for-tuples", + "lazy_static", + "mockall", "pallet-balances", "pallet-domains", "pallet-ownership", @@ -6194,6 +6198,7 @@ dependencies = [ name = "pallet-posts-tests" version = "0.2.2" dependencies = [ + "frame-benchmarking", "frame-support", "frame-system", "pallet-balances", @@ -11808,6 +11813,7 @@ dependencies = [ name = "subsocial-support" version = "0.2.2" dependencies = [ + "frame-benchmarking", "frame-support", "frame-system", "pallet-timestamp", diff --git a/integration-tests/Cargo.toml b/integration-tests/Cargo.toml index c19ee5dd..00a371a2 100644 --- a/integration-tests/Cargo.toml +++ b/integration-tests/Cargo.toml @@ -22,6 +22,7 @@ std = [ 'pallet-timestamp/std', 'frame-support/std', 'frame-system/std', + 'pallet-domains/std', 'pallet-permissions/std', 'pallet-posts/std', 'pallet-profiles/std', @@ -47,6 +48,7 @@ sp-runtime = { git = 'https://github.com/paritytech/substrate', branch = 'polkad sp-std = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } [dev-dependencies] +pallet-domains = { path = '../pallets/domains', default-features = false } pallet-permissions = { path = '../pallets/permissions', default-features = false } pallet-posts = { path = '../pallets/posts', default-features = false } pallet-profiles = { path = '../pallets/profiles', default-features = false } diff --git a/integration-tests/src/mock.rs b/integration-tests/src/mock.rs index a5ba3398..8a784f06 100644 --- a/integration-tests/src/mock.rs +++ b/integration-tests/src/mock.rs @@ -50,6 +50,7 @@ frame_support::construct_runtime!( SpaceFollows: pallet_space_follows, SpaceOwnership: pallet_ownership, Spaces: pallet_spaces, + Domains: pallet_domains, } ); @@ -163,7 +164,12 @@ impl pallet_space_follows::Config for TestRuntime { impl pallet_ownership::Config for TestRuntime { type RuntimeEvent = RuntimeEvent; type ProfileManager = Profiles; + type SpacesInterface = Spaces; + type SpacePermissionsProvider = Spaces; type CreatorStakingProvider = (); + type DomainsProvider = Domains; + type PostsProvider = Posts; + type Currency = Balances; type WeightInfo = (); } @@ -177,6 +183,30 @@ impl pallet_spaces::Config for TestRuntime { type WeightInfo = (); } +parameter_types! { + pub const RegistrationPeriodLimit: BlockNumber = 100; + pub const BaseDomainDeposit: u64 = 10; + pub const OuterValueByteDeposit: u64 = 1; + pub const InitialPaymentBeneficiary: AccountId = ACCOUNT1; + pub InitialPricesConfig: pallet_domains::types::PricesConfigVec = vec![(1, 100)]; +} + +impl pallet_domains::Config for TestRuntime { + type RuntimeEvent = RuntimeEvent; + type Currency = Balances; + type MinDomainLength = ConstU32<1>; + type MaxDomainLength = ConstU32<64>; + type MaxDomainsPerAccount = ConstU32<10>; + type DomainsInsertLimit = ConstU32<10>; + type RegistrationPeriodLimit = RegistrationPeriodLimit; + type MaxOuterValueLength = ConstU32<64>; + type BaseDomainDeposit = BaseDomainDeposit; + type OuterValueByteDeposit = OuterValueByteDeposit; + type InitialPaymentBeneficiary = InitialPaymentBeneficiary; + type InitialPricesConfig = InitialPricesConfig; + type WeightInfo = (); +} + pub(crate) type AccountId = u64; pub(crate) type BlockNumber = u64; diff --git a/integration-tests/src/tests/space_ownership.rs b/integration-tests/src/tests/space_ownership.rs index f9efe09d..c7a33de1 100644 --- a/integration-tests/src/tests/space_ownership.rs +++ b/integration-tests/src/tests/space_ownership.rs @@ -7,7 +7,7 @@ use frame_support::{assert_ok, assert_noop}; use sp_runtime::traits::Zero; -use pallet_ownership::Error as SpaceOwnershipError; +use pallet_ownership::{EntityWithOwnership, Error as SpaceOwnershipError}; use pallet_spaces::Error as SpacesError; use crate::mock::*; @@ -20,7 +20,7 @@ fn transfer_space_ownership_should_work() { assert_ok!(_transfer_default_space_ownership()); // Transfer SpaceId 1 owned by ACCOUNT1 to ACCOUNT2 assert_eq!( - SpaceOwnership::pending_space_owner(SPACE1).unwrap(), + SpaceOwnership::pending_ownership_transfer(EntityWithOwnership::Space(SPACE1)).unwrap(), ACCOUNT2 ); }); @@ -70,7 +70,7 @@ fn accept_pending_ownership_should_work() { assert_eq!(space.owner, ACCOUNT2); // Check that pending storage is cleared: - assert!(SpaceOwnership::pending_space_owner(SPACE1).is_none()); + assert!(SpaceOwnership::pending_ownership_transfer(EntityWithOwnership::Space(SPACE1)).is_none()); assert!(Balances::reserved_balance(ACCOUNT1).is_zero()); @@ -108,7 +108,7 @@ fn accept_pending_ownership_should_fail_if_origin_is_already_an_owner() { assert_noop!( _accept_pending_ownership(Some(RuntimeOrigin::signed(ACCOUNT1)), None), - SpaceOwnershipError::::AlreadyOwner + SpaceOwnershipError::::NotAllowedToAcceptOwnershipTransfer, ); }); } @@ -137,7 +137,7 @@ fn reject_pending_ownership_should_work() { assert_eq!(space.owner, ACCOUNT1); // Check whether storage state is correct - assert!(SpaceOwnership::pending_space_owner(SPACE1).is_none()); + assert!(SpaceOwnership::pending_ownership_transfer(EntityWithOwnership::Space(SPACE1)).is_none()); }); } @@ -153,7 +153,7 @@ fn reject_pending_ownership_should_work_when_proposal_rejected_by_current_space_ assert_eq!(space.owner, ACCOUNT1); // Check whether storage state is correct - assert!(SpaceOwnership::pending_space_owner(SPACE1).is_none()); + assert!(SpaceOwnership::pending_ownership_transfer(EntityWithOwnership::Space(SPACE1)).is_none()); }); } diff --git a/integration-tests/src/utils/space_ownership_utils.rs b/integration-tests/src/utils/space_ownership_utils.rs index de5119fc..a7da0a7f 100644 --- a/integration-tests/src/utils/space_ownership_utils.rs +++ b/integration-tests/src/utils/space_ownership_utils.rs @@ -5,6 +5,7 @@ // Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE use frame_support::pallet_prelude::*; +use pallet_ownership::EntityWithOwnership; use subsocial_support::SpaceId; @@ -22,7 +23,7 @@ pub(crate) fn _transfer_space_ownership( ) -> DispatchResult { SpaceOwnership::transfer_ownership( origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT1)), - space_id.unwrap_or(SPACE1), + space_id.map_or(EntityWithOwnership::Space(SPACE1), |id| EntityWithOwnership::Space(id)), transfer_to.unwrap_or(ACCOUNT2), ) } @@ -34,7 +35,7 @@ pub(crate) fn _accept_default_pending_ownership() -> DispatchResult { pub(crate) fn _accept_pending_ownership(origin: Option, space_id: Option) -> DispatchResult { SpaceOwnership::accept_pending_ownership( origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT2)), - space_id.unwrap_or(SPACE1), + space_id.map_or(EntityWithOwnership::Space(SPACE1), |id| EntityWithOwnership::Space(id)), ) } @@ -49,6 +50,6 @@ pub(crate) fn _reject_default_pending_ownership_by_current_owner() -> DispatchRe pub(crate) fn _reject_pending_ownership(origin: Option, space_id: Option) -> DispatchResult { SpaceOwnership::reject_pending_ownership( origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT2)), - space_id.unwrap_or(SPACE1), + space_id.map_or(EntityWithOwnership::Space(SPACE1), |id| EntityWithOwnership::Space(id)), ) } diff --git a/pallets/creator-staking/src/tests/mock.rs b/pallets/creator-staking/src/tests/mock.rs index bd81af82..32b4c26a 100644 --- a/pallets/creator-staking/src/tests/mock.rs +++ b/pallets/creator-staking/src/tests/mock.rs @@ -160,6 +160,8 @@ mock! { impl SpacesInterface for Spaces { fn get_space_owner(_space_id: SpaceId) -> Result; + + fn update_space_owner(_space_id: SpaceId, _new_owner: AccountId) -> DispatchResult; fn create_space(_owner: &AccountId, _content: Content) -> Result; } diff --git a/pallets/domains/Cargo.toml b/pallets/domains/Cargo.toml index 6cce0c44..b6b0b80d 100644 --- a/pallets/domains/Cargo.toml +++ b/pallets/domains/Cargo.toml @@ -33,7 +33,10 @@ sp-core = { git = "https://github.com/paritytech/substrate", branch = "polkadot- [features] default = ["std"] -runtime-benchmarks = ["frame-benchmarking"] +runtime-benchmarks = [ + "frame-benchmarking/runtime-benchmarks", + "subsocial-support/runtime-benchmarks", +] std = [ "codec/std", "scale-info/std", diff --git a/pallets/domains/src/lib.rs b/pallets/domains/src/lib.rs index f2410a2d..1b9eae48 100644 --- a/pallets/domains/src/lib.rs +++ b/pallets/domains/src/lib.rs @@ -775,5 +775,21 @@ pub mod pallet { Ok(()) } + + #[cfg(feature = "runtime-benchmarks")] + fn register_domain(owner: &T::AccountId, domain: &[u8]) -> Result, DispatchError> { + ensure!(domain.len() <= T::MaxDomainLength::get() as usize, Error::::DomainIsTooLong); + let domain_lc = Self::lower_domain_then_bound(domain); + + Self::do_register_domain( + owner.clone(), + owner.clone(), + domain_lc.clone(), + Content::None, + T::RegistrationPeriodLimit::get(), + )?; + + Ok(domain_lc.into()) + } } } diff --git a/pallets/posts/src/functions.rs b/pallets/posts/src/functions.rs index 54b544a7..8ffe52cc 100644 --- a/pallets/posts/src/functions.rs +++ b/pallets/posts/src/functions.rs @@ -476,4 +476,17 @@ impl PostsProvider for Pallet { Ok(()) } + + #[cfg(feature = "runtime-benchmarks")] + fn create_post(owner: &T::AccountId, space_id: SpaceId, content: Content) -> Result { + let new_post_id = Self::next_post_id(); + let new_post: Post = + Post::new(new_post_id, owner.clone(), Some(space_id), PostExtension::RegularPost, content.clone()); + + PostById::insert(new_post_id, new_post); + PostIdsBySpaceId::::mutate(space_id, |ids| ids.push(new_post_id)); + NextPostId::::mutate(|n| n.saturating_inc()); + + Ok(new_post_id) + } } diff --git a/pallets/posts/tests/Cargo.toml b/pallets/posts/tests/Cargo.toml index 4a94129f..daeb5a96 100644 --- a/pallets/posts/tests/Cargo.toml +++ b/pallets/posts/tests/Cargo.toml @@ -29,6 +29,7 @@ sp-runtime = { git = 'https://github.com/paritytech/substrate', branch = 'polkad sp-std = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } [dev-dependencies] +frame-benchmarking = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } sp-core = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } sp-io = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } pallet-balances = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.40", default-features = false } @@ -41,10 +42,12 @@ pallet-spaces = { default-features = false, path = '../../spaces' } [features] default = ['std'] +runtime-benchmarks = ['subsocial-support/runtime-benchmarks'] std = [ 'codec/std', 'scale-info/std', 'pallet-timestamp/std', + 'frame-benchmarking/std', 'frame-support/std', 'frame-system/std', 'sp-runtime/std', diff --git a/pallets/posts/tests/src/mock.rs b/pallets/posts/tests/src/mock.rs index e8fbd857..de59e1fd 100644 --- a/pallets/posts/tests/src/mock.rs +++ b/pallets/posts/tests/src/mock.rs @@ -5,12 +5,11 @@ // Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE use frame_support::{pallet_prelude::ConstU32, parameter_types, traits::Everything}; +use frame_support::dispatch::DispatchResult; use sp_core::H256; -use sp_runtime::{ - testing::Header, - traits::{BlakeTwo256, IdentityLookup}, -}; +use sp_runtime::{DispatchError, testing::Header, traits::{BlakeTwo256, IdentityLookup}}; use sp_std::convert::{TryFrom, TryInto}; +use subsocial_support::traits::DomainsProvider; use crate::tests_utils::*; @@ -32,7 +31,7 @@ frame_support::construct_runtime!( SpaceFollows: pallet_space_follows, Posts: pallet_posts, Spaces: pallet_spaces, - SpaceOwnership: pallet_ownership, + Ownership: pallet_ownership, } ); @@ -150,9 +149,39 @@ impl pallet_space_follows::Config for Test { type WeightInfo = (); } +parameter_types! { + pub const MaxDomainLength: u32 = 64; +} +pub struct MockEmptyDomainsProvider; +impl DomainsProvider for MockEmptyDomainsProvider { + type DomainLength = MaxDomainLength; + + fn get_domain_owner(_domain: &[u8]) -> Result { + Ok(ACCOUNT1) + } + + fn ensure_allowed_to_update_domain(_account: &AccountId, _domain: &[u8]) -> DispatchResult { + Ok(()) + } + + fn update_domain_owner(_domain: &[u8], _new_owner: &AccountId) -> DispatchResult { + Ok(()) + } + + #[cfg(feature = "runtime-benchmarks")] + fn register_domain(_owner: &AccountId, _domain: &[u8]) -> Result, DispatchError> { + Ok(Vec::new()) + } +} + impl pallet_ownership::Config for Test { type RuntimeEvent = RuntimeEvent; type ProfileManager = Profiles; + type SpacesInterface = Spaces; + type SpacePermissionsProvider = Spaces; type CreatorStakingProvider = (); + type DomainsProvider = MockEmptyDomainsProvider; + type PostsProvider = Posts; + type Currency = Balances; type WeightInfo = (); } diff --git a/pallets/posts/tests/src/tests_utils.rs b/pallets/posts/tests/src/tests_utils.rs index 77b7e5f6..16b60bf9 100644 --- a/pallets/posts/tests/src/tests_utils.rs +++ b/pallets/posts/tests/src/tests_utils.rs @@ -13,6 +13,7 @@ use std::{ use frame_support::{assert_ok, pallet_prelude::*}; use sp_core::storage::Storage; use sp_io::TestExternalities; +use pallet_ownership::EntityWithOwnership; use pallet_permissions::{SpacePermission as SP, SpacePermission, SpacePermissions}; use pallet_posts::{Comment, PostExtension, PostUpdate}; @@ -486,9 +487,9 @@ pub(crate) fn _transfer_space_ownership( space_id: Option, transfer_to: Option, ) -> DispatchResult { - SpaceOwnership::transfer_ownership( + Ownership::transfer_ownership( origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT1)), - space_id.unwrap_or(SPACE1), + space_id.map_or(EntityWithOwnership::Space(SPACE1), |id| EntityWithOwnership::Space(id)), transfer_to.unwrap_or(ACCOUNT2), ) } diff --git a/pallets/profiles/src/mock.rs b/pallets/profiles/src/mock.rs index 0f7f391f..a426e0cb 100644 --- a/pallets/profiles/src/mock.rs +++ b/pallets/profiles/src/mock.rs @@ -88,6 +88,8 @@ mock! { impl SpacesInterface for Spaces { fn get_space_owner(_space_id: SpaceId) -> Result; + + fn update_space_owner(_space_id: SpaceId, _new_owner: AccountId) -> DispatchResult; fn create_space(_owner: &AccountId, _content: Content) -> Result; } diff --git a/pallets/space-ownership/Cargo.toml b/pallets/space-ownership/Cargo.toml index 64e890d6..ed7b0157 100644 --- a/pallets/space-ownership/Cargo.toml +++ b/pallets/space-ownership/Cargo.toml @@ -12,7 +12,10 @@ categories = ['cryptography::cryptocurrencies'] [features] default = ['std'] -runtime-benchmarks = ['frame-benchmarking/runtime-benchmarks'] +runtime-benchmarks = [ + 'frame-benchmarking/runtime-benchmarks', + 'subsocial-support/runtime-benchmarks', +] std = [ 'codec/std', 'scale-info/std', @@ -40,3 +43,6 @@ frame-support = { git = 'https://github.com/paritytech/substrate', branch = 'pol frame-system = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } sp-runtime = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } sp-std = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } + +[dev-dependencies] +pallet-balances = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.40", default-features = false } diff --git a/pallets/space-ownership/src/benchmarking.rs b/pallets/space-ownership/src/benchmarking.rs index f8ad5991..5eef07a1 100644 --- a/pallets/space-ownership/src/benchmarking.rs +++ b/pallets/space-ownership/src/benchmarking.rs @@ -8,74 +8,153 @@ #![cfg(feature = "runtime-benchmarks")] -use super::*; use frame_benchmarking::{account, benchmarks}; -use frame_support::{dispatch::DispatchError, ensure}; +use frame_support::{dispatch::DispatchError, ensure, traits::Currency}; use frame_system::RawOrigin; -use pallet_spaces::types::Space; -use subsocial_support::Content; +use sp_runtime::traits::Bounded; -fn create_dummy_space( - origin: RawOrigin, -) -> Result, DispatchError> { - let space_id = pallet_spaces::NextSpaceId::::get(); +use subsocial_support::{ + traits::{DomainsProvider, PostsProvider, SpacesInterface}, + Content, +}; + +use super::*; - pallet_spaces::Pallet::::create_space(origin.clone().into(), Content::None, None)?; +type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; - let space = pallet_spaces::SpaceById::::get(space_id) - .ok_or(DispatchError::Other("Space not found"))?; +fn grab_accounts() -> (T::AccountId, T::AccountId) { + let acc1 = account::("Acc1", 1, 0); + let acc2 = account::("Acc2", 2, 0); + T::Currency::make_free_balance_be(&acc1, BalanceOf::::max_value()); + T::Currency::make_free_balance_be(&acc2, BalanceOf::::max_value()); - Ok(space) + (acc1, acc2) +} + +fn create_dummy_space( + owner: &T::AccountId, +) -> Result, DispatchError> { + let space_id = T::SpacesInterface::create_space(owner, Content::None)?; + Ok(EntityWithOwnership::Space(space_id)) +} + +fn create_dummy_post( + owner: &T::AccountId, +) -> Result, DispatchError> { + let space_id = T::SpacesInterface::create_space(owner, Content::None)?; + let post_id = T::PostsProvider::create_post(owner, space_id, Content::None)?; + Ok(EntityWithOwnership::Post(post_id)) +} + +fn create_dummy_domain( + owner: &T::AccountId, +) -> Result, DispatchError> { + let domain = T::DomainsProvider::register_domain(owner, "dappforce.sub".as_bytes())?; + let domain_bounded = domain.try_into().unwrap(); + Ok(EntityWithOwnership::Domain(domain_bounded)) } benchmarks! { transfer_space_ownership { - let acc1 = account::("Acc1", 1, 0); - let acc2 = account::("Acc2", 2, 0); + let (acc1, acc2) = grab_accounts::(); + let space_entity = create_dummy_space::(&acc1)?; + }: transfer_ownership(RawOrigin::Signed(acc1.clone()), space_entity.clone(), acc2.clone()) + verify { + ensure!( + PendingOwnershipTransfers::::get(&space_entity) == Some(acc2), + "Request was not created", + ); + } + + transfer_post_ownership { + let (acc1, acc2) = grab_accounts::(); + let post_entity = create_dummy_post::(&acc1)?; + }: transfer_ownership(RawOrigin::Signed(acc1.clone()), post_entity.clone(), acc2.clone()) + verify { + ensure!( + PendingOwnershipTransfers::::get(&post_entity) == Some(acc2), + "Request was not created", + ); + } - let space = create_dummy_space::(RawOrigin::Signed(acc1.clone()))?; - }: _(RawOrigin::Signed(acc1.clone()), space.id, acc2.clone()) + transfer_domain_ownership { + let (acc1, acc2) = grab_accounts::(); + let domain_entity = create_dummy_domain::(&acc1)?; + }: transfer_ownership(RawOrigin::Signed(acc1), domain_entity.clone(), acc2.clone()) verify { - ensure!(PendingSpaceOwner::::get(&space.id) == Some(acc2), "Request is not found"); + ensure!( + PendingOwnershipTransfers::::get(&domain_entity) == Some(acc2), + "Request was not created", + ); } - accept_pending_ownership { - let acc1 = account::("Acc1", 1, 0); - let acc2 = account::("Acc2", 2, 0); + accept_pending_space_ownership_transfer { + let (acc1, acc2) = grab_accounts::(); + let space_entity = create_dummy_space::(&acc1)?; - let space = create_dummy_space::(RawOrigin::Signed(acc1.clone()))?; Pallet::::transfer_ownership( - RawOrigin::Signed(acc1.clone()).into(), - space.id, + RawOrigin::Signed(acc1).into(), + space_entity.clone(), acc2.clone(), )?; - }: _(RawOrigin::Signed(acc2.clone()), space.id) + }: accept_pending_ownership(RawOrigin::Signed(acc2), space_entity.clone()) verify { - let space = pallet_spaces::SpaceById::::get(space.id) - .ok_or(DispatchError::Other("Space not found"))?; + ensure!( + PendingOwnershipTransfers::::get(&space_entity).is_none(), + "Request was not cleaned", + ); + } - ensure!(PendingSpaceOwner::::get(&space.id) == None, "Request was not cleaned"); - ensure!(space.owner == acc2, "Space owner is not updated"); + accept_pending_post_ownership_transfer { + let (acc1, acc2) = grab_accounts::(); + let post_entity = create_dummy_post::(&acc1)?; + + Pallet::::transfer_ownership( + RawOrigin::Signed(acc1).into(), + post_entity.clone(), + acc2.clone(), + )?; + }: accept_pending_ownership(RawOrigin::Signed(acc2), post_entity.clone()) + verify { + ensure!( + PendingOwnershipTransfers::::get(&post_entity).is_none(), + "Request was not cleaned", + ); } - reject_pending_ownership { - let acc1 = account::("Acc1", 1, 0); - let acc2 = account::("Acc2", 2, 0); + accept_pending_domain_ownership_transfer { + let (acc1, acc2) = grab_accounts::(); + let domain_entity = create_dummy_domain::(&acc1)?; - let space = create_dummy_space::(RawOrigin::Signed(acc1.clone()))?; Pallet::::transfer_ownership( - RawOrigin::Signed(acc1.clone()).into(), - space.id, + RawOrigin::Signed(acc1).into(), + domain_entity.clone(), acc2.clone(), )?; - }: _(RawOrigin::Signed(acc2.clone()), space.id) + }: accept_pending_ownership(RawOrigin::Signed(acc2), domain_entity.clone()) verify { - let space = pallet_spaces::SpaceById::::get(space.id) - .ok_or(DispatchError::Other("Space not found"))?; + ensure!( + PendingOwnershipTransfers::::get(&domain_entity).is_none(), + "Request was not cleaned", + ); + } + + reject_pending_ownership { + let (acc1, acc2) = grab_accounts::(); + let space_entity = create_dummy_space::(&acc1)?; - ensure!(PendingSpaceOwner::::get(&space.id) == None, "Request was not cleaned"); - ensure!(space.owner == acc1, "Space owner is updated"); + Pallet::::transfer_ownership( + RawOrigin::Signed(acc1).into(), + space_entity.clone(), + acc2.clone(), + )?; + }: _(RawOrigin::Signed(acc2), space_entity.clone()) + verify { + ensure!( + PendingOwnershipTransfers::::get(&space_entity).is_none(), + "Request was not cleaned", + ); } } diff --git a/pallets/space-ownership/src/lib.rs b/pallets/space-ownership/src/lib.rs index 434ce341..b05faf0a 100644 --- a/pallets/space-ownership/src/lib.rs +++ b/pallets/space-ownership/src/lib.rs @@ -56,6 +56,8 @@ pub mod pallet { type PostsProvider: PostsProvider; + type Currency: frame_support::traits::Currency; + type WeightInfo: WeightInfo; } @@ -70,8 +72,6 @@ pub mod pallet { pub enum Error { /// The current entity owner cannot transfer ownership to themselves. CannotTransferToCurrentOwner, - /// Account is already an owner of an entity. - AlreadyOwner, /// Cannot transfer ownership, because a space is registered as an active creator. ActiveCreatorCannotTransferOwnership, /// There is no pending ownership transfer for a given entity. @@ -108,7 +108,13 @@ pub mod pallet { #[pallet::call] impl Pallet { #[pallet::call_index(0)] - #[pallet::weight(::WeightInfo::transfer_space_ownership())] + #[pallet::weight( + match entity { + EntityWithOwnership::Space(_) => T::WeightInfo::transfer_space_ownership(), + EntityWithOwnership::Post(_) => T::WeightInfo::transfer_post_ownership(), + EntityWithOwnership::Domain(_) => T::WeightInfo::transfer_domain_ownership(), + } + )] pub fn transfer_ownership( origin: OriginFor, entity: EntityWithOwnership, @@ -146,7 +152,13 @@ pub mod pallet { } #[pallet::call_index(1)] - #[pallet::weight(::WeightInfo::accept_pending_ownership())] + #[pallet::weight( + match entity { + EntityWithOwnership::Space(_) => T::WeightInfo::accept_pending_space_ownership_transfer(), + EntityWithOwnership::Post(_) => T::WeightInfo::accept_pending_post_ownership_transfer(), + EntityWithOwnership::Domain(_) => T::WeightInfo::accept_pending_domain_ownership_transfer(), + } + )] pub fn accept_pending_ownership(origin: OriginFor, entity: EntityWithOwnership) -> DispatchResult { let new_owner = ensure_signed(origin)?; diff --git a/pallets/space-ownership/src/weights.rs b/pallets/space-ownership/src/weights.rs index c0eac355..f0bfd6b0 100644 --- a/pallets/space-ownership/src/weights.rs +++ b/pallets/space-ownership/src/weights.rs @@ -1,41 +1,28 @@ -// Copyright (C) DAPPFORCE PTE. LTD. -// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0. -// -// Full notice is available at https://github.com/dappforce/subsocial-parachain/blob/main/COPYRIGHT -// Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE - //! Autogenerated weights for pallet_ownership //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev -//! DATE: 2023-02-15, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]` -//! HOSTNAME: `benchmarks-ci`, CPU: `Intel(R) Xeon(R) Platinum 8280 CPU @ 2.70GHz` +//! DATE: 2024-02-29, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! WORST CASE MAP SIZE: `1000000` +//! HOSTNAME: `MacBook-Pro-Vladislav.local`, CPU: `` //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 1024 // Executed Command: - // ./scripts/../target/release/subsocial-collator - // benchmark - // pallet - // --chain - // dev - // --execution - // wasm - // --wasm-execution - // Compiled - // --pallet - // pallet_ownership - // --extrinsic - // * - // --steps - // 50 - // --repeat - // 20 - // --heap-pages - // 4096 - // --output - // pallets/space-ownership/src/weights.rs - // --template - // ./.maintain/weight-template.hbs +// ./scripts/../target/release/subsocial-collator +// benchmark +// pallet +// --chain=dev +// --steps=50 +// --repeat=20 +// --pallet +// pallet_ownership +// --extrinsic +// * +// --execution=wasm +// --wasm-execution=Compiled +// --heap-pages=4096 +// --output=pallets/space-ownership/src/weights.rs +// --template=./.maintain/weight-template.hbs #![cfg_attr(rustfmt, rustfmt_skip)] #![allow(unused_parens)] @@ -48,67 +35,233 @@ use sp_std::marker::PhantomData; /// Weight functions needed for pallet_ownership. pub trait WeightInfo { fn transfer_space_ownership() -> Weight; - fn accept_pending_ownership() -> Weight; + fn transfer_post_ownership() -> Weight; + fn transfer_domain_ownership() -> Weight; + fn accept_pending_space_ownership_transfer() -> Weight; + fn accept_pending_post_ownership_transfer() -> Weight; + fn accept_pending_domain_ownership_transfer() -> Weight; fn reject_pending_ownership() -> Weight; } /// Weights for pallet_ownership using the Substrate node and recommended hardware. pub struct SubstrateWeight(PhantomData); - impl WeightInfo for SubstrateWeight { - // Storage: Spaces SpaceById (r:1 w:0) - // Storage: SpaceOwnership PendingSpaceOwner (r:0 w:1) - fn transfer_space_ownership() -> Weight { - // Minimum execution time: 36_442 nanoseconds. - Weight::from_parts(36_970_000, 0) - .saturating_add(T::DbWeight::get().reads(1)) - .saturating_add(T::DbWeight::get().writes(1)) - } - // Storage: Spaces SpaceById (r:1 w:1) - // Storage: SpaceOwnership PendingSpaceOwner (r:1 w:1) - // Storage: Spaces SpaceIdsByOwner (r:2 w:2) - // Storage: Profiles ProfileSpaceIdByAccount (r:1 w:0) - fn accept_pending_ownership() -> Weight { - // Minimum execution time: 60_105 nanoseconds. - Weight::from_parts(60_893_000, 0) - .saturating_add(T::DbWeight::get().reads(5)) - .saturating_add(T::DbWeight::get().writes(4)) - } - // Storage: Spaces SpaceById (r:1 w:0) - // Storage: SpaceOwnership PendingSpaceOwner (r:1 w:1) - fn reject_pending_ownership() -> Weight { - // Minimum execution time: 40_600 nanoseconds. - Weight::from_parts(41_309_000, 0) - .saturating_add(T::DbWeight::get().reads(2)) - .saturating_add(T::DbWeight::get().writes(1)) - } +impl WeightInfo for SubstrateWeight { + /// Storage: Spaces SpaceById (r:1 w:0) + /// Proof Skipped: Spaces SpaceById (max_values: None, max_size: None, mode: Measured) + /// Storage: CreatorStaking RegisteredCreators (r:1 w:0) + /// Proof: CreatorStaking RegisteredCreators (max_values: None, max_size: Some(53), added: 2528, mode: MaxEncodedLen) + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:0 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + fn transfer_space_ownership() -> Weight { + // Proof Size summary in bytes: + // Measured: `1447` + // Estimated: `8430` + // Minimum execution time: 20_000_000 picoseconds. + Weight::from_parts(21_000_000, 8430) + .saturating_add(T::DbWeight::get().reads(2_u64)) + .saturating_add(T::DbWeight::get().writes(1_u64)) + } + /// Storage: Posts PostById (r:1 w:0) + /// Proof Skipped: Posts PostById (max_values: None, max_size: None, mode: Measured) + /// Storage: Spaces SpaceById (r:1 w:0) + /// Proof Skipped: Spaces SpaceById (max_values: None, max_size: None, mode: Measured) + /// Storage: SpaceFollows SpaceFollowedByAccount (r:1 w:0) + /// Proof Skipped: SpaceFollows SpaceFollowedByAccount (max_values: None, max_size: None, mode: Measured) + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:0 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + fn transfer_post_ownership() -> Weight { + // Proof Size summary in bytes: + // Measured: `1700` + // Estimated: `15495` + // Minimum execution time: 27_000_000 picoseconds. + Weight::from_parts(28_000_000, 15495) + .saturating_add(T::DbWeight::get().reads(3_u64)) + .saturating_add(T::DbWeight::get().writes(1_u64)) + } + /// Storage: Domains RegisteredDomains (r:1 w:0) + /// Proof Skipped: Domains RegisteredDomains (max_values: None, max_size: None, mode: Measured) + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:0 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + fn transfer_domain_ownership() -> Weight { + // Proof Size summary in bytes: + // Measured: `282` + // Estimated: `3747` + // Minimum execution time: 15_000_000 picoseconds. + Weight::from_parts(16_000_000, 3747) + .saturating_add(T::DbWeight::get().reads(1_u64)) + .saturating_add(T::DbWeight::get().writes(1_u64)) + } + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:1 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + /// Storage: Spaces SpaceById (r:1 w:1) + /// Proof Skipped: Spaces SpaceById (max_values: None, max_size: None, mode: Measured) + /// Storage: CreatorStaking RegisteredCreators (r:1 w:0) + /// Proof: CreatorStaking RegisteredCreators (max_values: None, max_size: Some(53), added: 2528, mode: MaxEncodedLen) + /// Storage: Spaces SpaceIdsByOwner (r:2 w:2) + /// Proof Skipped: Spaces SpaceIdsByOwner (max_values: None, max_size: None, mode: Measured) + /// Storage: Profiles ProfileSpaceIdByAccount (r:1 w:0) + /// Proof: Profiles ProfileSpaceIdByAccount (max_values: None, max_size: Some(56), added: 2531, mode: MaxEncodedLen) + fn accept_pending_space_ownership_transfer() -> Weight { + // Proof Size summary in bytes: + // Measured: `1638` + // Estimated: `23290` + // Minimum execution time: 36_000_000 picoseconds. + Weight::from_parts(37_000_000, 23290) + .saturating_add(T::DbWeight::get().reads(6_u64)) + .saturating_add(T::DbWeight::get().writes(4_u64)) + } + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:1 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + /// Storage: Posts PostById (r:1 w:1) + /// Proof Skipped: Posts PostById (max_values: None, max_size: None, mode: Measured) + fn accept_pending_post_ownership_transfer() -> Weight { + // Proof Size summary in bytes: + // Measured: `403` + // Estimated: `7438` + // Minimum execution time: 20_000_000 picoseconds. + Weight::from_parts(20_000_000, 7438) + .saturating_add(T::DbWeight::get().reads(2_u64)) + .saturating_add(T::DbWeight::get().writes(2_u64)) + } + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:1 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + /// Storage: Domains RegisteredDomains (r:1 w:1) + /// Proof Skipped: Domains RegisteredDomains (max_values: None, max_size: None, mode: Measured) + /// Storage: Domains DomainsByOwner (r:2 w:2) + /// Proof Skipped: Domains DomainsByOwner (max_values: None, max_size: None, mode: Measured) + /// Storage: System Account (r:2 w:2) + /// Proof: System Account (max_values: None, max_size: Some(128), added: 2603, mode: MaxEncodedLen) + fn accept_pending_domain_ownership_transfer() -> Weight { + // Proof Size summary in bytes: + // Measured: `688` + // Estimated: `20547` + // Minimum execution time: 56_000_000 picoseconds. + Weight::from_parts(57_000_000, 20547) + .saturating_add(T::DbWeight::get().reads(6_u64)) + .saturating_add(T::DbWeight::get().writes(6_u64)) + } + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:1 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + /// Storage: Spaces SpaceById (r:1 w:0) + /// Proof Skipped: Spaces SpaceById (max_values: None, max_size: None, mode: Measured) + fn reject_pending_ownership() -> Weight { + // Proof Size summary in bytes: + // Measured: `1595` + // Estimated: `8630` + // Minimum execution time: 21_000_000 picoseconds. + Weight::from_parts(22_000_000, 8630) + .saturating_add(T::DbWeight::get().reads(2_u64)) + .saturating_add(T::DbWeight::get().writes(1_u64)) } +} - // For backwards compatibility and tests - impl WeightInfo for () { - // Storage: Spaces SpaceById (r:1 w:0) - // Storage: SpaceOwnership PendingSpaceOwner (r:0 w:1) - fn transfer_space_ownership() -> Weight { - // Minimum execution time: 36_442 nanoseconds. - Weight::from_parts(36_970_000, 0) - .saturating_add(RocksDbWeight::get().reads(1)) - .saturating_add(RocksDbWeight::get().writes(1)) - } - // Storage: Spaces SpaceById (r:1 w:1) - // Storage: SpaceOwnership PendingSpaceOwner (r:1 w:1) - // Storage: Spaces SpaceIdsByOwner (r:2 w:2) - // Storage: Profiles ProfileSpaceIdByAccount (r:1 w:0) - fn accept_pending_ownership() -> Weight { - // Minimum execution time: 60_105 nanoseconds. - Weight::from_parts(60_893_000, 0) - .saturating_add(RocksDbWeight::get().reads(5)) - .saturating_add(RocksDbWeight::get().writes(4)) - } - // Storage: Spaces SpaceById (r:1 w:0) - // Storage: SpaceOwnership PendingSpaceOwner (r:1 w:1) - fn reject_pending_ownership() -> Weight { - // Minimum execution time: 40_600 nanoseconds. - Weight::from_parts(41_309_000, 0) - .saturating_add(RocksDbWeight::get().reads(2)) - .saturating_add(RocksDbWeight::get().writes(1)) - } +// For backwards compatibility and tests +impl WeightInfo for () { + /// Storage: Spaces SpaceById (r:1 w:0) + /// Proof Skipped: Spaces SpaceById (max_values: None, max_size: None, mode: Measured) + /// Storage: CreatorStaking RegisteredCreators (r:1 w:0) + /// Proof: CreatorStaking RegisteredCreators (max_values: None, max_size: Some(53), added: 2528, mode: MaxEncodedLen) + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:0 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + fn transfer_space_ownership() -> Weight { + // Proof Size summary in bytes: + // Measured: `1447` + // Estimated: `8430` + // Minimum execution time: 20_000_000 picoseconds. + Weight::from_parts(21_000_000, 8430) + .saturating_add(RocksDbWeight::get().reads(2_u64)) + .saturating_add(RocksDbWeight::get().writes(1_u64)) + } + /// Storage: Posts PostById (r:1 w:0) + /// Proof Skipped: Posts PostById (max_values: None, max_size: None, mode: Measured) + /// Storage: Spaces SpaceById (r:1 w:0) + /// Proof Skipped: Spaces SpaceById (max_values: None, max_size: None, mode: Measured) + /// Storage: SpaceFollows SpaceFollowedByAccount (r:1 w:0) + /// Proof Skipped: SpaceFollows SpaceFollowedByAccount (max_values: None, max_size: None, mode: Measured) + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:0 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + fn transfer_post_ownership() -> Weight { + // Proof Size summary in bytes: + // Measured: `1700` + // Estimated: `15495` + // Minimum execution time: 27_000_000 picoseconds. + Weight::from_parts(28_000_000, 15495) + .saturating_add(RocksDbWeight::get().reads(3_u64)) + .saturating_add(RocksDbWeight::get().writes(1_u64)) + } + /// Storage: Domains RegisteredDomains (r:1 w:0) + /// Proof Skipped: Domains RegisteredDomains (max_values: None, max_size: None, mode: Measured) + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:0 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + fn transfer_domain_ownership() -> Weight { + // Proof Size summary in bytes: + // Measured: `282` + // Estimated: `3747` + // Minimum execution time: 15_000_000 picoseconds. + Weight::from_parts(16_000_000, 3747) + .saturating_add(RocksDbWeight::get().reads(1_u64)) + .saturating_add(RocksDbWeight::get().writes(1_u64)) + } + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:1 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + /// Storage: Spaces SpaceById (r:1 w:1) + /// Proof Skipped: Spaces SpaceById (max_values: None, max_size: None, mode: Measured) + /// Storage: CreatorStaking RegisteredCreators (r:1 w:0) + /// Proof: CreatorStaking RegisteredCreators (max_values: None, max_size: Some(53), added: 2528, mode: MaxEncodedLen) + /// Storage: Spaces SpaceIdsByOwner (r:2 w:2) + /// Proof Skipped: Spaces SpaceIdsByOwner (max_values: None, max_size: None, mode: Measured) + /// Storage: Profiles ProfileSpaceIdByAccount (r:1 w:0) + /// Proof: Profiles ProfileSpaceIdByAccount (max_values: None, max_size: Some(56), added: 2531, mode: MaxEncodedLen) + fn accept_pending_space_ownership_transfer() -> Weight { + // Proof Size summary in bytes: + // Measured: `1638` + // Estimated: `23290` + // Minimum execution time: 36_000_000 picoseconds. + Weight::from_parts(37_000_000, 23290) + .saturating_add(RocksDbWeight::get().reads(6_u64)) + .saturating_add(RocksDbWeight::get().writes(4_u64)) + } + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:1 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + /// Storage: Posts PostById (r:1 w:1) + /// Proof Skipped: Posts PostById (max_values: None, max_size: None, mode: Measured) + fn accept_pending_post_ownership_transfer() -> Weight { + // Proof Size summary in bytes: + // Measured: `403` + // Estimated: `7438` + // Minimum execution time: 20_000_000 picoseconds. + Weight::from_parts(20_000_000, 7438) + .saturating_add(RocksDbWeight::get().reads(2_u64)) + .saturating_add(RocksDbWeight::get().writes(2_u64)) + } + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:1 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + /// Storage: Domains RegisteredDomains (r:1 w:1) + /// Proof Skipped: Domains RegisteredDomains (max_values: None, max_size: None, mode: Measured) + /// Storage: Domains DomainsByOwner (r:2 w:2) + /// Proof Skipped: Domains DomainsByOwner (max_values: None, max_size: None, mode: Measured) + /// Storage: System Account (r:2 w:2) + /// Proof: System Account (max_values: None, max_size: Some(128), added: 2603, mode: MaxEncodedLen) + fn accept_pending_domain_ownership_transfer() -> Weight { + // Proof Size summary in bytes: + // Measured: `688` + // Estimated: `20547` + // Minimum execution time: 56_000_000 picoseconds. + Weight::from_parts(57_000_000, 20547) + .saturating_add(RocksDbWeight::get().reads(6_u64)) + .saturating_add(RocksDbWeight::get().writes(6_u64)) + } + /// Storage: SpaceOwnership PendingOwnershipTransfers (r:1 w:1) + /// Proof: SpaceOwnership PendingOwnershipTransfers (max_values: None, max_size: Some(105), added: 2580, mode: MaxEncodedLen) + /// Storage: Spaces SpaceById (r:1 w:0) + /// Proof Skipped: Spaces SpaceById (max_values: None, max_size: None, mode: Measured) + fn reject_pending_ownership() -> Weight { + // Proof Size summary in bytes: + // Measured: `1595` + // Estimated: `8630` + // Minimum execution time: 21_000_000 picoseconds. + Weight::from_parts(22_000_000, 8630) + .saturating_add(RocksDbWeight::get().reads(2_u64)) + .saturating_add(RocksDbWeight::get().writes(1_u64)) } +} \ No newline at end of file diff --git a/pallets/space-ownership/tests/Cargo.toml b/pallets/space-ownership/tests/Cargo.toml index 24098b54..ce5fcc2c 100644 --- a/pallets/space-ownership/tests/Cargo.toml +++ b/pallets/space-ownership/tests/Cargo.toml @@ -31,6 +31,9 @@ sp-runtime = { git = 'https://github.com/paritytech/substrate', branch = 'polkad sp-std = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } [dev-dependencies] +mockall = '0.11.3' +lazy_static = '1.4.0' + sp-core = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } sp-io = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } pallet-balances = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.40", default-features = false } diff --git a/pallets/space-ownership/tests/src/mock.rs b/pallets/space-ownership/tests/src/mock.rs index bd648045..8b96428e 100644 --- a/pallets/space-ownership/tests/src/mock.rs +++ b/pallets/space-ownership/tests/src/mock.rs @@ -5,12 +5,19 @@ // Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE use frame_support::{pallet_prelude::ConstU32, parameter_types, traits::Everything}; +use lazy_static::lazy_static; +use mockall::mock; use sp_core::H256; use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup}, }; -use sp_std::convert::{TryFrom, TryInto}; +use sp_std::{ + convert::{TryFrom, TryInto}, + sync::{Mutex, MutexGuard}, +}; + +use subsocial_support::{traits::CreatorStakingProvider, SpaceId}; type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; @@ -28,7 +35,7 @@ frame_support::construct_runtime!( Roles: pallet_roles, Profiles: pallet_profiles, SpaceFollows: pallet_space_follows, - SpaceOwnership: pallet_ownership, + Ownership: pallet_ownership, Spaces: pallet_spaces, Domains: pallet_domains, Posts: pallet_posts, @@ -41,6 +48,29 @@ pub(super) type BlockNumber = u64; pub(crate) const DOMAIN_DEPOSIT: Balance = 10; +// Mocks + +// CreatorStakingProvider +mock! { + // This will generate MockSpaces + pub CreatorStaking {} + impl CreatorStakingProvider for CreatorStaking { + fn is_creator_active(creator_id: SpaceId) -> bool; + } +} + +lazy_static! { + static ref MTX: Mutex<()> = Mutex::new(()); +} + +// mockall crate requires synchronized access for the mocking of static methods. +pub(super) fn use_static_mock() -> MutexGuard<'static, ()> { + match MTX.lock() { + Ok(guard) => guard, + Err(poisoned) => poisoned.into_inner(), + } +} + parameter_types! { pub const BlockHashCount: u64 = 250; pub const SS58Prefix: u8 = 42; @@ -148,18 +178,19 @@ impl pallet_posts::Config for Test { } parameter_types! { + pub const MaxDomainLength: u32 = 64; pub const BaseDomainDeposit: Balance = DOMAIN_DEPOSIT; pub const OuterValueByteDeposit: Balance = 5; pub const RegistrationPeriodLimit: BlockNumber = 5; pub const InitialPaymentBeneficiary: AccountId = 1; - pub InitialPricesConfig: pallet_domains::types::PricesConfigVec = Vec::new(); + pub InitialPricesConfig: pallet_domains::types::PricesConfigVec = vec![(1, 100)]; } impl pallet_domains::Config for Test { type RuntimeEvent = RuntimeEvent; type Currency = Balances; type MinDomainLength = ConstU32<1>; - type MaxDomainLength = ConstU32<64>; + type MaxDomainLength = MaxDomainLength; type MaxDomainsPerAccount = ConstU32<5>; type DomainsInsertLimit = ConstU32<5>; type RegistrationPeriodLimit = RegistrationPeriodLimit; @@ -176,8 +207,9 @@ impl pallet_ownership::Config for Test { type ProfileManager = Profiles; type SpacesInterface = Spaces; type SpacePermissionsProvider = Spaces; - type CreatorStakingProvider = (); + type CreatorStakingProvider = MockCreatorStaking; type DomainsProvider = Domains; type PostsProvider = Posts; + type Currency = Balances; type WeightInfo = (); } diff --git a/pallets/space-ownership/tests/src/tests.rs b/pallets/space-ownership/tests/src/tests.rs index 5cec2043..155b69c6 100644 --- a/pallets/space-ownership/tests/src/tests.rs +++ b/pallets/space-ownership/tests/src/tests.rs @@ -5,169 +5,183 @@ // Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE use frame_support::{assert_noop, assert_ok}; -use sp_runtime::traits::Zero; -use pallet_ownership::Error as SpaceOwnershipError; -use pallet_spaces::Error as SpacesError; +use pallet_ownership::{EntityWithOwnership, Error as OwnershipError, Event as OwnershipEvent}; use crate::{mock::*, tests_utils::*}; #[test] -fn transfer_space_ownership_should_work() { - ExtBuilder::build_with_space().execute_with(|| { - assert_ok!(_transfer_default_space_ownership()); // Transfer SpaceId 1 owned by ACCOUNT1 to ACCOUNT2 - - assert_eq!(SpaceOwnership::pending_space_owner(SPACE1).unwrap(), ACCOUNT2); - }); +fn transfer_ownership_works() { + ExtBuilder::build_with_pending_transfers(); } #[test] -fn transfer_space_ownership_should_fail_when_space_not_found() { - ExtBuilder::build().execute_with(|| { - assert_noop!(_transfer_default_space_ownership(), SpacesError::::SpaceNotFound); +fn accept_pending_ownership_works() { + ExtBuilder::build_with_pending_transfers().execute_with(|| { + let _m = use_static_mock(); + let creator_staking_ctx = MockCreatorStaking::is_creator_active_context(); + + // `is_creator_active` should return `false`. + creator_staking_ctx.expect().returning(|_| false).once(); + + // Mock accepting ownership transfer for a space entity + assert_ok!(Ownership::accept_pending_ownership( + RuntimeOrigin::signed(ACCOUNT2), + default_space_entity(), + )); + + // Mock accepting ownership transfer for a post entity + assert_ok!(Ownership::accept_pending_ownership( + RuntimeOrigin::signed(ACCOUNT2), + default_post_entity(), + )); + + // Mock accepting ownership transfer for a domain entity + assert_ok!(Ownership::accept_pending_ownership( + RuntimeOrigin::signed(ACCOUNT2), + default_domain_entity(), + )); + + System::assert_has_event(OwnershipEvent::OwnershipTransferAccepted { + account: ACCOUNT2, + entity: default_space_entity(), + }.into()); + + System::assert_has_event(OwnershipEvent::OwnershipTransferAccepted { + account: ACCOUNT2, + entity: default_post_entity(), + }.into()); + + System::assert_has_event(OwnershipEvent::OwnershipTransferAccepted { + account: ACCOUNT2, + entity: default_domain_entity(), + }.into()); }); } #[test] -fn transfer_space_ownership_should_fail_when_account_is_not_space_owner() { - ExtBuilder::build_with_space().execute_with(|| { - assert_noop!( - _transfer_space_ownership(Some(RuntimeOrigin::signed(ACCOUNT2)), None, Some(ACCOUNT1)), - SpacesError::::NotASpaceOwner - ); +fn reject_pending_ownership_works() { + ExtBuilder::build_with_pending_transfers().execute_with(|| { + // Rejecting ownership transfer by the current owner + assert_reject_transfers_ok(ACCOUNT1); + + // Re-create the pending transfers + assert_create_transfers_ok(); + + // Rejecting ownership transfer by a target account + assert_reject_transfers_ok(ACCOUNT2); }); } #[test] -fn transfer_space_ownership_should_fail_when_trying_to_transfer_to_current_owner() { - ExtBuilder::build_with_space().execute_with(|| { +fn transfer_ownership_should_not_allow_transfer_to_current_owner() { + ExtBuilder::build_with_all_enitities().execute_with(|| { + // Mock a transfer from account 1 to itself for a space entity assert_noop!( - _transfer_space_ownership(Some(RuntimeOrigin::signed(ACCOUNT1)), None, Some(ACCOUNT1)), - SpaceOwnershipError::::CannotTransferToCurrentOwner + Ownership::transfer_ownership( + RuntimeOrigin::signed(ACCOUNT1), + EntityWithOwnership::Space(SPACE1), + ACCOUNT1 + ), + OwnershipError::::CannotTransferToCurrentOwner ); }); } #[test] -fn accept_pending_ownership_should_work() { - ExtBuilder::build_with_space().execute_with(|| { - // Transfer SpaceId 1 owned by ACCOUNT1 to ACCOUNT2: - assert_ok!(_transfer_default_space_ownership()); - - // Account 2 accepts the transfer of ownership: - assert_ok!(_accept_default_pending_ownership()); - - // Check that Account 2 is a new space owner: - let space = Spaces::space_by_id(SPACE1).unwrap(); - assert_eq!(space.owner, ACCOUNT2); - - // Check that pending storage is cleared: - assert!(SpaceOwnership::pending_space_owner(SPACE1).is_none()); +fn transfer_ownership_should_not_allow_active_creator_to_transfer_space_ownership() { + ExtBuilder::build_with_all_enitities().execute_with(|| { + let _m = use_static_mock(); + let creator_staking_ctx = MockCreatorStaking::is_creator_active_context(); - assert!(Balances::reserved_balance(ACCOUNT1).is_zero()); + // `is_creator_active` should return `true`. + creator_staking_ctx.expect().returning(|_| true).once(); - // assert_eq!(Balances::reserved_balance(ACCOUNT2), HANDLE_DEPOSIT); - }); -} - -#[test] -fn accept_pending_ownership_should_fail_when_space_not_found() { - ExtBuilder::build_with_pending_ownership_transfer_no_space().execute_with(|| { - assert_noop!(_accept_default_pending_ownership(), SpacesError::::SpaceNotFound); - }); -} - -#[test] -fn accept_pending_ownership_should_fail_when_no_pending_transfer_for_space() { - ExtBuilder::build_with_space().execute_with(|| { + // Mock a transfer from an active creator (creator is active in this test) assert_noop!( - _accept_default_pending_ownership(), - SpaceOwnershipError::::NoPendingTransfer + Ownership::transfer_ownership( + RuntimeOrigin::signed(ACCOUNT1), + EntityWithOwnership::Space(SPACE1), + ACCOUNT2 + ), + OwnershipError::::ActiveCreatorCannotTransferOwnership ); }); } #[test] -fn accept_pending_ownership_should_fail_if_origin_is_already_an_owner() { - ExtBuilder::build_with_space().execute_with(|| { - assert_ok!(_transfer_default_space_ownership()); - +fn accept_pending_ownership_should_fail_if_no_pending_transfer() { + ExtBuilder::build_with_all_enitities().execute_with(|| { + // Mock accepting ownership transfer for a space entity with no pending transfer assert_noop!( - _accept_pending_ownership(Some(RuntimeOrigin::signed(ACCOUNT1)), None), - SpaceOwnershipError::::AlreadyOwner + Ownership::accept_pending_ownership( + RuntimeOrigin::signed(ACCOUNT2), + EntityWithOwnership::Space(SPACE1) + ), + OwnershipError::::NoPendingTransfer ); }); } #[test] -fn accept_pending_ownership_should_fail_if_origin_is_not_equal_to_pending_account() { - ExtBuilder::build_with_space().execute_with(|| { - assert_ok!(_transfer_default_space_ownership()); +fn accept_pending_ownership_should_not_allow_non_target_to_accept() { + ExtBuilder::build_with_pending_transfers().execute_with(|| { + let _m = use_static_mock(); + let creator_staking_ctx = MockCreatorStaking::is_creator_active_context(); + // `is_creator_active` should return `true` once. + creator_staking_ctx.expect().returning(|_| false).once(); + + // Mock accepting ownership transfer for a space entity by a non-owner assert_noop!( - _accept_pending_ownership(Some(RuntimeOrigin::signed(ACCOUNT3)), None), - SpaceOwnershipError::::NotAllowedToAcceptOwnershipTransfer + Ownership::accept_pending_ownership( + RuntimeOrigin::signed(ACCOUNT3), + EntityWithOwnership::Space(SPACE1) + ), + OwnershipError::::NotAllowedToAcceptOwnershipTransfer ); - }); -} - -#[test] -fn reject_pending_ownership_should_work() { - ExtBuilder::build_with_space().execute_with(|| { - assert_ok!(_transfer_default_space_ownership()); - // Transfer SpaceId 1 owned by ACCOUNT1 to ACCOUNT2 - assert_ok!(_reject_default_pending_ownership()); // Rejecting a transfer from ACCOUNT2 - - // Check whether owner was not changed - let space = Spaces::space_by_id(SPACE1).unwrap(); - assert_eq!(space.owner, ACCOUNT1); - - // Check whether storage state is correct - assert!(SpaceOwnership::pending_space_owner(SPACE1).is_none()); - }); -} - -#[test] -fn reject_pending_ownership_should_work_when_proposal_rejected_by_current_space_owner() { - ExtBuilder::build_with_space().execute_with(|| { - assert_ok!(_transfer_default_space_ownership()); - // Transfer SpaceId 1 owned by ACCOUNT1 to ACCOUNT2 - assert_ok!(_reject_default_pending_ownership_by_current_owner()); // Rejecting a transfer from ACCOUNT2 - - // Check whether owner was not changed - let space = Spaces::space_by_id(SPACE1).unwrap(); - assert_eq!(space.owner, ACCOUNT1); - - // Check whether storage state is correct - assert!(SpaceOwnership::pending_space_owner(SPACE1).is_none()); - }); -} -#[test] -fn reject_pending_ownership_should_fail_when_space_not_found() { - ExtBuilder::build_with_pending_ownership_transfer_no_space().execute_with(|| { - assert_noop!(_reject_default_pending_ownership(), SpacesError::::SpaceNotFound); + assert_ok!( + Ownership::accept_pending_ownership( + RuntimeOrigin::signed(ACCOUNT2), + EntityWithOwnership::Space(SPACE1) + ) + ); }); } #[test] -fn reject_pending_ownership_should_fail_when_no_pending_transfer_on_space() { - ExtBuilder::build_with_space().execute_with(|| { +fn reject_pending_ownership_should_fail_if_no_pending_transfer() { + ExtBuilder::build_with_all_enitities().execute_with(|| { + // Mock rejecting ownership transfer for a space entity with no pending transfer assert_noop!( - _reject_default_pending_ownership(), - SpaceOwnershipError::::NoPendingTransfer - ); // Rejecting a transfer from ACCOUNT2 + Ownership::reject_pending_ownership( + RuntimeOrigin::signed(ACCOUNT1), + EntityWithOwnership::Space(SPACE1) + ), + OwnershipError::::NoPendingTransfer + ); }); } #[test] -fn reject_pending_ownership_should_fail_when_account_is_not_allowed_to_reject() { - ExtBuilder::build_with_space().execute_with(|| { - assert_ok!(_transfer_default_space_ownership()); // Transfer SpaceId 1 owned by ACCOUNT1 to ACCOUNT2 - +fn reject_pending_ownership_should_not_allow_ineligible_account_to_reject() { + ExtBuilder::build_with_pending_transfers().execute_with(|| { + // Mock rejecting ownership transfer for a space entity by a non-owner assert_noop!( - _reject_pending_ownership(Some(RuntimeOrigin::signed(ACCOUNT3)), None), - SpaceOwnershipError::::NotAllowedToRejectOwnershipTransfer - ); // Rejecting a transfer from ACCOUNT2 + Ownership::reject_pending_ownership( + RuntimeOrigin::signed(ACCOUNT3), + EntityWithOwnership::Space(SPACE1) + ), + OwnershipError::::NotAllowedToRejectOwnershipTransfer + ); + + // Mock rejecting ownership transfer for a space entity by the owner + assert_ok!( + Ownership::reject_pending_ownership( + RuntimeOrigin::signed(ACCOUNT1), + EntityWithOwnership::Space(SPACE1) + ) + ); }); } diff --git a/pallets/space-ownership/tests/src/tests_utils.rs b/pallets/space-ownership/tests/src/tests_utils.rs index 5fa698c2..d4b67590 100644 --- a/pallets/space-ownership/tests/src/tests_utils.rs +++ b/pallets/space-ownership/tests/src/tests_utils.rs @@ -7,10 +7,12 @@ use frame_support::{assert_ok, pallet_prelude::*}; use sp_core::storage::Storage; use sp_io::TestExternalities; +use pallet_ownership::{EntityWithOwnership, Event as OwnershipEvent}; -use pallet_permissions::SpacePermissions; +use pallet_posts::PostExtension; use pallet_spaces::*; -use subsocial_support::{Content, SpaceId}; +use subsocial_support::{Content, PostId, SpaceId}; +use subsocial_support::mock_functions::valid_content_ipfs; use crate::mock::*; @@ -26,7 +28,7 @@ impl ExtBuilder { } let _ = pallet_balances::GenesisConfig:: { - balances: accounts.iter().cloned().map(|k| (k, 100)).collect(), + balances: accounts.iter().cloned().map(|k| (k, 1000)).collect(), } .assimilate_storage(storage); } @@ -38,109 +40,218 @@ impl ExtBuilder { Self::configure_storages(&mut storage); let mut ext = TestExternalities::from(storage); - ext.execute_with(|| System::set_block_number(1)); + ext.execute_with(|| { + let _m = use_static_mock(); + System::set_block_number(1); + }); ext } - fn add_default_space() { + pub fn add_default_space() { assert_ok!(_create_default_space()); } - /// Custom ext configuration with SpaceId 1 and BlockNumber 1 - pub fn build_with_space() -> TestExternalities { + pub fn add_default_post(space_id_opt: Option) { + assert_ok!(_create_default_post(space_id_opt)); + } + + pub fn add_default_domain() { + assert_ok!(_create_default_domain()); + } + + /// Custom ext configuration with SpaceId 1001, PostId 1 and Domain "dappforce.sub" + pub fn build_with_all_enitities() -> TestExternalities { let mut ext = Self::build(); - ext.execute_with(Self::add_default_space); + ext.execute_with(|| { + Self::add_default_space(); + let space_id = NextSpaceId::::get().saturating_sub(1); + Self::add_default_post(Some(space_id)); + Self::add_default_domain(); + }); ext } - /// Custom ext configuration with pending ownership transfer without Space - pub fn build_with_pending_ownership_transfer_no_space() -> TestExternalities { - let mut ext = Self::build_with_space(); + /// Custom ext configuration with SpaceId 1001, PostId 1 and Domain "dappforce.sub" + /// and pending transfers for all entities (current owner: `ACCOUNT1`, new owner: `ACCOUNT2`) + pub fn build_with_pending_transfers() -> TestExternalities { + let mut ext = Self::build_with_all_enitities(); ext.execute_with(|| { - assert_ok!(_transfer_default_space_ownership()); - >::remove(SPACE1); + assert_create_transfers_ok(); }); ext } } -////// Consts +// Constants pub(crate) const ACCOUNT1: AccountId = 1; pub(crate) const ACCOUNT2: AccountId = 2; pub(crate) const ACCOUNT3: AccountId = 3; pub(crate) const SPACE1: SpaceId = 1001; +pub(crate) const POST1: PostId = 1; -///////////// Space Utils +// Other pallets utils -pub(crate) fn space_content_ipfs() -> Content { - Content::IPFS(b"bafyreib3mgbou4xln42qqcgj6qlt3cif35x4ribisxgq7unhpun525l54e".to_vec()) +// Data + +pub(crate) fn default_domain() -> BoundedVec { + "dappforce.sub".as_bytes().to_vec().try_into().unwrap() } -pub(crate) fn _create_default_space() -> DispatchResult { - _create_space(None, None, None) +pub(crate) fn default_domain_entity() -> EntityWithOwnership { + EntityWithOwnership::Domain(default_domain()) } -pub(crate) fn _create_space( - origin: Option, - content: Option, - permissions: Option>, -) -> DispatchResult { +pub(crate) const fn default_space_entity() -> EntityWithOwnership { + EntityWithOwnership::Space(SPACE1) +} + +pub(crate) const fn default_post_entity() -> EntityWithOwnership { + EntityWithOwnership::Post(POST1) +} + +// Extrinsics + +pub(crate) fn _create_default_space() -> DispatchResult { Spaces::create_space( - origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT1)), - content.unwrap_or_else(space_content_ipfs), - permissions.unwrap_or_default(), + RuntimeOrigin::signed(ACCOUNT1), + valid_content_ipfs(), + None, ) } -//////// Space ownership utils - -pub(crate) fn _transfer_default_space_ownership() -> DispatchResult { - _transfer_space_ownership(None, None, None) +pub(crate) fn _create_default_post(space_id_opt: Option) -> DispatchResult { + Posts::create_post( + RuntimeOrigin::signed(ACCOUNT1), + space_id_opt, + PostExtension::RegularPost, + valid_content_ipfs(), + ) } -pub(crate) fn _transfer_space_ownership( - origin: Option, - space_id: Option, - transfer_to: Option, -) -> DispatchResult { - SpaceOwnership::transfer_ownership( - origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT1)), - space_id.unwrap_or(SPACE1), - transfer_to.unwrap_or(ACCOUNT2), +pub(crate) fn _create_default_domain() -> DispatchResult { + Domains::register_domain( + RuntimeOrigin::signed(ACCOUNT1), + None, + default_domain(), + Content::None, + RegistrationPeriodLimit::get(), ) } -pub(crate) fn _accept_default_pending_ownership() -> DispatchResult { - _accept_pending_ownership(None, None) +// Space ownership utils + +pub(crate) fn _transfer_ownership( + account: AccountId, + entity: EntityWithOwnership, + transfer_to: AccountId, +) -> DispatchResult { + Ownership::transfer_ownership( + RuntimeOrigin::signed(account), + entity, + transfer_to, + ) } pub(crate) fn _accept_pending_ownership( - origin: Option, - space_id: Option, + account: AccountId, + entity: EntityWithOwnership, ) -> DispatchResult { - SpaceOwnership::accept_pending_ownership( - origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT2)), - space_id.unwrap_or(SPACE1), + Ownership::accept_pending_ownership( + RuntimeOrigin::signed(account), + entity, ) } -pub(crate) fn _reject_default_pending_ownership() -> DispatchResult { - _reject_pending_ownership(None, None) +pub(crate) fn _reject_pending_ownership( + account: AccountId, + entity: EntityWithOwnership, +) -> DispatchResult { + Ownership::reject_pending_ownership( + RuntimeOrigin::signed(account), + entity, + ) } -pub(crate) fn _reject_default_pending_ownership_by_current_owner() -> DispatchResult { - _reject_pending_ownership(Some(RuntimeOrigin::signed(ACCOUNT1)), None) +pub(crate) fn assert_create_transfers_ok() { + let _m = use_static_mock(); + // `is_creator_active` should return `is_active`. + let creator_staking_ctx = MockCreatorStaking::is_creator_active_context(); + creator_staking_ctx.expect().returning(|_| false).once(); + + // Mock a transfer from account 1 to account 2 for a space entity + assert_ok!(Ownership::transfer_ownership( + RuntimeOrigin::signed(ACCOUNT1), + EntityWithOwnership::Space(SPACE1), + ACCOUNT2 + )); + + // Mock a transfer from account 1 to account 2 for a post entity + assert_ok!(Ownership::transfer_ownership( + RuntimeOrigin::signed(ACCOUNT1), + EntityWithOwnership::Post(POST1), + ACCOUNT2 + )); + + // Mock a transfer from account 1 to account 2 for a domain entity + assert_ok!(Ownership::transfer_ownership( + RuntimeOrigin::signed(ACCOUNT1), + EntityWithOwnership::Domain(default_domain()), + ACCOUNT2 + )); + + System::assert_has_event(OwnershipEvent::OwnershipTransferCreated { + current_owner: ACCOUNT1, + entity: default_space_entity(), + new_owner: ACCOUNT2, + }.into()); + + System::assert_has_event(OwnershipEvent::OwnershipTransferCreated { + current_owner: ACCOUNT1, + entity: default_post_entity(), + new_owner: ACCOUNT2, + }.into()); + + System::assert_has_event(OwnershipEvent::OwnershipTransferCreated { + current_owner: ACCOUNT1, + entity: default_domain_entity(), + new_owner: ACCOUNT2, + }.into()); } -pub(crate) fn _reject_pending_ownership( - origin: Option, - space_id: Option, -) -> DispatchResult { - SpaceOwnership::reject_pending_ownership( - origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT2)), - space_id.unwrap_or(SPACE1), - ) +pub(crate) fn assert_reject_transfers_ok(account: AccountId) { + // Mock rejecting ownership transfer for a space entity + assert_ok!(Ownership::reject_pending_ownership( + RuntimeOrigin::signed(account), + default_space_entity(), + )); + + // Mock rejecting ownership transfer for a post entity + assert_ok!(Ownership::reject_pending_ownership( + RuntimeOrigin::signed(account), + default_post_entity(), + )); + + // Mock rejecting ownership transfer for a domain entity + assert_ok!(Ownership::reject_pending_ownership( + RuntimeOrigin::signed(account), + default_domain_entity(), + )); + + System::assert_has_event(OwnershipEvent::OwnershipTransferRejected { + account, + entity: default_space_entity(), + }.into()); + + System::assert_has_event(OwnershipEvent::OwnershipTransferRejected { + account, + entity: default_post_entity(), + }.into()); + + System::assert_has_event(OwnershipEvent::OwnershipTransferRejected { + account, + entity: default_domain_entity(), + }.into()); } diff --git a/pallets/support/Cargo.toml b/pallets/support/Cargo.toml index 04c443d9..bb10b005 100644 --- a/pallets/support/Cargo.toml +++ b/pallets/support/Cargo.toml @@ -12,11 +12,13 @@ categories = ["cryptography::cryptocurrencies"] [features] default = ["std"] +runtime-benchmarks = ['frame-benchmarking/runtime-benchmarks'] std = [ "serde/std", "strum/std", "codec/std", "scale-info/std", + "frame-benchmarking/std", "frame-support/std", "frame-system/std", "pallet-timestamp/std", @@ -31,6 +33,7 @@ codec = { package = "parity-scale-codec", version = "3.0.0", default-features = scale-info = { version = "2.2.0", default-features = false, features = ["derive"] } # Substrate dependencies +frame-benchmarking = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false, optional = true } frame-support = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.40", default-features = false } frame-system = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.40", default-features = false } pallet-timestamp = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.40", default-features = false } diff --git a/pallets/support/src/traits/common.rs b/pallets/support/src/traits/common.rs index 5a4b46fa..45cd7ac3 100644 --- a/pallets/support/src/traits/common.rs +++ b/pallets/support/src/traits/common.rs @@ -64,6 +64,9 @@ pub trait DomainsProvider { fn ensure_allowed_to_update_domain(account: &AccountId, domain: &[u8]) -> DispatchResult; fn update_domain_owner(domain: &[u8], new_owner: &AccountId) -> DispatchResult; + + #[cfg(feature = "runtime-benchmarks")] + fn register_domain(owner: &AccountId, domain: &[u8]) -> Result, DispatchError>; } pub trait PostsProvider { @@ -72,4 +75,7 @@ pub trait PostsProvider { fn ensure_allowed_to_update_post(account: &AccountId, post_id: PostId) -> DispatchResult; fn update_post_owner(post_id: PostId, new_owner: &AccountId) -> DispatchResult; + + #[cfg(feature = "runtime-benchmarks")] + fn create_post(owner: &AccountId, space_id: SpaceId, content: Content) -> Result; } diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 173ce6ed..3afa0ad5 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -778,6 +778,7 @@ impl pallet_ownership::Config for Runtime { type CreatorStakingProvider = CreatorStaking; type DomainsProvider = Domains; type PostsProvider = Posts; + type Currency = Balances; type WeightInfo = pallet_ownership::weights::SubstrateWeight; } From 05587d20ce091a84b78a65fd71860b805923ba83 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Thu, 14 Mar 2024 13:17:56 +0000 Subject: [PATCH 15/25] Add missing license headers --- pallets/space-ownership/src/weights.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pallets/space-ownership/src/weights.rs b/pallets/space-ownership/src/weights.rs index f0bfd6b0..b3d1ad04 100644 --- a/pallets/space-ownership/src/weights.rs +++ b/pallets/space-ownership/src/weights.rs @@ -1,3 +1,9 @@ +// Copyright (C) DAPPFORCE PTE. LTD. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0. +// +// Full notice is available at https://github.com/dappforce/subsocial-parachain/blob/main/COPYRIGHT +// Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE + //! Autogenerated weights for pallet_ownership //! From 901a75c71c60bcb532e06ffc3a6eea9e0730e4c2 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Thu, 14 Mar 2024 14:42:37 +0200 Subject: [PATCH 16/25] Add migration from old to new ownership pallet --- Cargo.lock | 1 + pallets/space-ownership/Cargo.toml | 2 + pallets/space-ownership/src/migration.rs | 78 +++++++++++++++++++++--- runtime/src/lib.rs | 19 ++++-- 4 files changed, 87 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 729399e1..5e3d046c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6088,6 +6088,7 @@ dependencies = [ "pallet-permissions", "parity-scale-codec", "scale-info", + "sp-io", "sp-runtime", "sp-std", "subsocial-support", diff --git a/pallets/space-ownership/Cargo.toml b/pallets/space-ownership/Cargo.toml index ed7b0157..c1c095d3 100644 --- a/pallets/space-ownership/Cargo.toml +++ b/pallets/space-ownership/Cargo.toml @@ -22,6 +22,7 @@ std = [ 'frame-benchmarking/std', 'frame-support/std', 'frame-system/std', + 'sp-io/std', 'sp-runtime/std', 'sp-std/std', 'pallet-permissions/std', @@ -41,6 +42,7 @@ subsocial-support = { default-features = false, path = '../support' } frame-benchmarking = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false, optional = true } frame-support = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } frame-system = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } +sp-io = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } sp-runtime = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } sp-std = { git = 'https://github.com/paritytech/substrate', branch = 'polkadot-v0.9.40', default-features = false } diff --git a/pallets/space-ownership/src/migration.rs b/pallets/space-ownership/src/migration.rs index 225f6f72..cc308d70 100644 --- a/pallets/space-ownership/src/migration.rs +++ b/pallets/space-ownership/src/migration.rs @@ -5,7 +5,6 @@ // Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE use frame_support::{log, traits::OnRuntimeUpgrade}; -use sp_runtime::Saturating; #[cfg(feature = "try-runtime")] use sp_runtime::traits::Zero; #[cfg(feature = "try-runtime")] @@ -16,7 +15,14 @@ use super::*; const LOG_TARGET: &'static str = "runtime::ownership"; pub mod v1 { - use frame_support::{pallet_prelude::*, storage_alias, weights::Weight}; + use frame_support::{ + migration::move_pallet, pallet_prelude::*, storage_alias, weights::Weight, + }; + #[cfg(feature = "try-runtime")] + use frame_support::migration::storage_key_iter; + #[cfg(feature = "try-runtime")] + use sp_io::hashing::twox_128; + use sp_runtime::Saturating; use subsocial_support::SpaceId; @@ -26,13 +32,18 @@ pub mod v1 { pub type PendingSpaceOwner = StorageMap, Twox64Concat, SpaceId, ::AccountId>; - pub struct MigrateToV1(sp_std::marker::PhantomData); + pub struct MigrateToV1(sp_std::marker::PhantomData<(T, P, N)>); - impl OnRuntimeUpgrade for MigrateToV1 { + impl> OnRuntimeUpgrade + for MigrateToV1 + { fn on_runtime_upgrade() -> Weight { let current_version = Pallet::::current_storage_version(); let onchain_version = Pallet::::on_chain_storage_version(); + let old_pallet_name = N::get(); + let new_pallet_name =

::name(); + log::info!( target: LOG_TARGET, "Running migration with current storage version {:?} / onchain {:?}", @@ -41,6 +52,18 @@ pub mod v1 { ); if onchain_version == 0 && current_version == 1 { + current_version.put::>(); + + if new_pallet_name == old_pallet_name { + log::warn!( + target: LOG_TARGET, + "new ownership name is equal to the old one, only bumping the version" + ); + return T::DbWeight::get().reads_writes(1, 1) + } + + move_pallet(old_pallet_name.as_bytes(), new_pallet_name.as_bytes()); + let mut migrated = 0u64; for (space_id, account) in PendingSpaceOwner::::drain() { @@ -51,19 +74,18 @@ pub mod v1 { migrated.saturating_inc(); } - current_version.put::>(); - log::info!( target: LOG_TARGET, "Upgraded {} records, storage to version {:?}", migrated, current_version ); - T::DbWeight::get().reads_writes(migrated + 1, migrated + 1) + + ::BlockWeights::get().max_block } else { log::info!( target: LOG_TARGET, - "Migration did not execute. This probably should be removed" + "Migration did not execute. v1 upgrade should be removed" ); T::DbWeight::get().reads(1) } @@ -73,8 +95,17 @@ pub mod v1 { fn pre_upgrade() -> Result, &'static str> { let current_version = Pallet::::current_storage_version(); let onchain_version = Pallet::::on_chain_storage_version(); + let old_pallet_name = N::get().as_bytes(); + let old_pallet_prefix = twox_128(old_pallet_name); ensure!(onchain_version == 0 && current_version == 1, "migration from version 0 to 1."); - let prev_count = PendingSpaceOwner::::iter().count(); + + ensure!( + sp_io::storage::next_key(&old_pallet_prefix).is_some(), + "no data for the old pallet name has been detected" + ); + + // let prev_count = PendingSpaceOwner::::iter().count(); + let prev_count = storage_key_iter::(old_pallet_name, b"PendingSpaceOwner").count(); Ok((prev_count as u32).encode()) } @@ -86,6 +117,9 @@ pub mod v1 { let post_count = PendingOwnershipTransfers::::iter().count() as u32; let old_storage_count = PendingSpaceOwner::::iter().count(); + let old_pallet_name = N::get(); + let new_pallet_name =

::name(); + ensure!( prev_count == post_count, "the records count before and after the migration should be the same" @@ -94,6 +128,32 @@ pub mod v1 { ensure!(Pallet::::on_chain_storage_version() == 1, "wrong storage version"); + // skip storage prefix checks for the same pallet names + if new_pallet_name == old_pallet_name { + return Ok(()); + } + + // Assert that nothing remains at the old prefix. + let old_pallet_prefix = twox_128(N::get().as_bytes()); + let old_pallet_prefix_iter = frame_support::storage::KeyPrefixIterator::new( + old_pallet_prefix.to_vec(), + old_pallet_prefix.to_vec(), + |_| Ok(()), + ); + ensure!( + old_pallet_prefix_iter.count().is_zero(), + "old pallet data hasn't been removed" + ); + + // NOTE: storage_version_key is already in the new prefix. + let new_pallet_prefix = twox_128(new_pallet_name.as_bytes()); + let new_pallet_prefix_iter = frame_support::storage::KeyPrefixIterator::new( + new_pallet_prefix.to_vec(), + new_pallet_prefix.to_vec(), + |_| Ok(()), + ); + assert_eq!(new_pallet_prefix_iter.count(), (prev_count + 1) as usize); + Ok(()) } } diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 3afa0ad5..af36d3b3 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -121,9 +121,20 @@ pub type Executive = frame_executive::Executive< frame_system::ChainContext, Runtime, AllPalletsWithSystem, - pallet_ownership::migration::v1::MigrateToV1, + pallet_ownership::migration::v1::MigrateToV1< + Runtime, + Ownership, + OwnershipMigrationV1OldPallet, + >, >; +pub struct OwnershipMigrationV1OldPallet; +impl frame_support::traits::Get<&'static str> for OwnershipMigrationV1OldPallet { + fn get() -> &'static str { + "SpaceOwnership" + } +} + /// Handles converting a weight scalar to a fee value, based on the scale and granularity of the /// node's balance type. /// @@ -592,7 +603,7 @@ impl InstanceFilter for ProxyType { ProxyType::Management => matches!( c, RuntimeCall::Spaces(..) - | RuntimeCall::SpaceOwnership(..) + | RuntimeCall::Ownership(..) | RuntimeCall::Roles(..) | RuntimeCall::Profiles(..) | RuntimeCall::Domains(..) @@ -911,7 +922,7 @@ construct_runtime!( AccountFollows: pallet_account_follows = 72, Profiles: pallet_profiles = 73, SpaceFollows: pallet_space_follows = 74, - SpaceOwnership: pallet_ownership = 75, + Ownership: pallet_ownership = 75, Spaces: pallet_spaces = 76, PostFollows: pallet_post_follows = 77, Posts: pallet_posts = 78, @@ -942,7 +953,7 @@ mod benches { [pallet_reactions, Reactions] [pallet_roles, Roles] [pallet_space_follows, SpaceFollows] - [pallet_ownership, SpaceOwnership] + [pallet_ownership, Ownership] [pallet_spaces, Spaces] [pallet_post_follows, PostFollows] [pallet_posts, Posts] From 18df14eead08e6d0315d4431e458352daf1e2690 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Wed, 20 Mar 2024 17:41:02 +0200 Subject: [PATCH 17/25] Update spec_version to 42 --- runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 8ce4ef98..4d6e1ff2 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -189,7 +189,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: create_runtime_str!("subsocial-parachain"), impl_name: create_runtime_str!("subsocial-parachain"), authoring_version: 1, - spec_version: 41, + spec_version: 42, impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 9, From 7728026932cc24c1d7f6079ee5ba4ba896ccae1d Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Fri, 22 Mar 2024 13:34:22 +0200 Subject: [PATCH 18/25] Apply suggestions after a code review Co-Authored-By: Alex Siman --- integration-tests/src/mock.rs | 4 +- .../src/tests/space_ownership.rs | 14 ++-- .../src/utils/space_ownership_utils.rs | 8 +-- pallets/creator-staking/src/lib.rs | 6 +- pallets/creator-staking/src/tests/mock.rs | 6 +- pallets/domains/src/lib.rs | 8 +-- pallets/posts/src/functions.rs | 5 ++ pallets/posts/src/lib.rs | 4 -- pallets/posts/tests/src/mock.rs | 4 +- pallets/posts/tests/src/tests_utils.rs | 4 +- pallets/profiles/src/benchmarking.rs | 4 +- pallets/profiles/src/lib.rs | 6 +- pallets/profiles/src/mock.rs | 6 +- pallets/roles/src/benchmarking.rs | 2 +- pallets/space-ownership/src/benchmarking.rs | 18 ++--- pallets/space-ownership/src/lib.rs | 68 +++++++++---------- pallets/space-ownership/src/migration.rs | 2 +- pallets/space-ownership/tests/src/mock.rs | 4 +- pallets/space-ownership/tests/src/tests.rs | 20 +++--- .../space-ownership/tests/src/tests_utils.rs | 26 +++---- pallets/spaces/src/lib.rs | 4 +- pallets/spaces/tests/src/mock.rs | 2 +- pallets/support/src/traits.rs | 2 +- pallets/support/src/traits/common.rs | 3 +- runtime/src/lib.rs | 6 +- 25 files changed, 117 insertions(+), 119 deletions(-) diff --git a/integration-tests/src/mock.rs b/integration-tests/src/mock.rs index 8a784f06..d652b35e 100644 --- a/integration-tests/src/mock.rs +++ b/integration-tests/src/mock.rs @@ -133,7 +133,7 @@ impl pallet_posts::Config for TestRuntime { impl pallet_profiles::Config for TestRuntime { type RuntimeEvent = RuntimeEvent; type SpacePermissionsProvider = Spaces; - type SpacesInterface = Spaces; + type SpacesProvider = Spaces; type WeightInfo = (); } @@ -164,7 +164,7 @@ impl pallet_space_follows::Config for TestRuntime { impl pallet_ownership::Config for TestRuntime { type RuntimeEvent = RuntimeEvent; type ProfileManager = Profiles; - type SpacesInterface = Spaces; + type SpacesProvider = Spaces; type SpacePermissionsProvider = Spaces; type CreatorStakingProvider = (); type DomainsProvider = Domains; diff --git a/integration-tests/src/tests/space_ownership.rs b/integration-tests/src/tests/space_ownership.rs index c7a33de1..f696550d 100644 --- a/integration-tests/src/tests/space_ownership.rs +++ b/integration-tests/src/tests/space_ownership.rs @@ -7,7 +7,7 @@ use frame_support::{assert_ok, assert_noop}; use sp_runtime::traits::Zero; -use pallet_ownership::{EntityWithOwnership, Error as SpaceOwnershipError}; +use pallet_ownership::{OwnableEntity, Error as SpaceOwnershipError}; use pallet_spaces::Error as SpacesError; use crate::mock::*; @@ -20,7 +20,7 @@ fn transfer_space_ownership_should_work() { assert_ok!(_transfer_default_space_ownership()); // Transfer SpaceId 1 owned by ACCOUNT1 to ACCOUNT2 assert_eq!( - SpaceOwnership::pending_ownership_transfer(EntityWithOwnership::Space(SPACE1)).unwrap(), + SpaceOwnership::pending_ownership_transfer(OwnableEntity::Space(SPACE1)).unwrap(), ACCOUNT2 ); }); @@ -70,7 +70,7 @@ fn accept_pending_ownership_should_work() { assert_eq!(space.owner, ACCOUNT2); // Check that pending storage is cleared: - assert!(SpaceOwnership::pending_ownership_transfer(EntityWithOwnership::Space(SPACE1)).is_none()); + assert!(SpaceOwnership::pending_ownership_transfer(OwnableEntity::Space(SPACE1)).is_none()); assert!(Balances::reserved_balance(ACCOUNT1).is_zero()); @@ -108,7 +108,7 @@ fn accept_pending_ownership_should_fail_if_origin_is_already_an_owner() { assert_noop!( _accept_pending_ownership(Some(RuntimeOrigin::signed(ACCOUNT1)), None), - SpaceOwnershipError::::NotAllowedToAcceptOwnershipTransfer, + SpaceOwnershipError::::CurrentOwnerCannotAcceptOwnershipTransfer, ); }); } @@ -120,7 +120,7 @@ fn accept_pending_ownership_should_fail_if_origin_is_not_equal_to_pending_accoun assert_noop!( _accept_pending_ownership(Some(RuntimeOrigin::signed(ACCOUNT3)), None), - SpaceOwnershipError::::NotAllowedToAcceptOwnershipTransfer + SpaceOwnershipError::::CurrentOwnerCannotAcceptOwnershipTransfer ); }); } @@ -137,7 +137,7 @@ fn reject_pending_ownership_should_work() { assert_eq!(space.owner, ACCOUNT1); // Check whether storage state is correct - assert!(SpaceOwnership::pending_ownership_transfer(EntityWithOwnership::Space(SPACE1)).is_none()); + assert!(SpaceOwnership::pending_ownership_transfer(OwnableEntity::Space(SPACE1)).is_none()); }); } @@ -153,7 +153,7 @@ fn reject_pending_ownership_should_work_when_proposal_rejected_by_current_space_ assert_eq!(space.owner, ACCOUNT1); // Check whether storage state is correct - assert!(SpaceOwnership::pending_ownership_transfer(EntityWithOwnership::Space(SPACE1)).is_none()); + assert!(SpaceOwnership::pending_ownership_transfer(OwnableEntity::Space(SPACE1)).is_none()); }); } diff --git a/integration-tests/src/utils/space_ownership_utils.rs b/integration-tests/src/utils/space_ownership_utils.rs index a7da0a7f..49aa955d 100644 --- a/integration-tests/src/utils/space_ownership_utils.rs +++ b/integration-tests/src/utils/space_ownership_utils.rs @@ -5,7 +5,7 @@ // Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE use frame_support::pallet_prelude::*; -use pallet_ownership::EntityWithOwnership; +use pallet_ownership::OwnableEntity; use subsocial_support::SpaceId; @@ -23,7 +23,7 @@ pub(crate) fn _transfer_space_ownership( ) -> DispatchResult { SpaceOwnership::transfer_ownership( origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT1)), - space_id.map_or(EntityWithOwnership::Space(SPACE1), |id| EntityWithOwnership::Space(id)), + space_id.map_or(OwnableEntity::Space(SPACE1), |id| OwnableEntity::Space(id)), transfer_to.unwrap_or(ACCOUNT2), ) } @@ -35,7 +35,7 @@ pub(crate) fn _accept_default_pending_ownership() -> DispatchResult { pub(crate) fn _accept_pending_ownership(origin: Option, space_id: Option) -> DispatchResult { SpaceOwnership::accept_pending_ownership( origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT2)), - space_id.map_or(EntityWithOwnership::Space(SPACE1), |id| EntityWithOwnership::Space(id)), + space_id.map_or(OwnableEntity::Space(SPACE1), |id| OwnableEntity::Space(id)), ) } @@ -50,6 +50,6 @@ pub(crate) fn _reject_default_pending_ownership_by_current_owner() -> DispatchRe pub(crate) fn _reject_pending_ownership(origin: Option, space_id: Option) -> DispatchResult { SpaceOwnership::reject_pending_ownership( origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT2)), - space_id.map_or(EntityWithOwnership::Space(SPACE1), |id| EntityWithOwnership::Space(id)), + space_id.map_or(OwnableEntity::Space(SPACE1), |id| OwnableEntity::Space(id)), ) } diff --git a/pallets/creator-staking/src/lib.rs b/pallets/creator-staking/src/lib.rs index d22f3abc..5fd772e2 100644 --- a/pallets/creator-staking/src/lib.rs +++ b/pallets/creator-staking/src/lib.rs @@ -30,7 +30,7 @@ pub mod pallet { use sp_runtime::{traits::Zero, Perbill, Saturating}; use pallet_permissions::SpacePermissionsInfoOf; - use subsocial_support::{traits::{SpacesInterface, SpacePermissionsProvider}, SpaceId}; + use subsocial_support::{traits::{SpacesProvider, SpacePermissionsProvider}, SpaceId}; pub use crate::types::*; @@ -54,7 +54,7 @@ pub mod pallet { type Currency: LockableCurrency + ReservableCurrency; - type SpacesInterface: SpacesInterface; + type SpacesProvider: SpacesProvider; type SpacePermissionsProvider: SpacePermissionsProvider< Self::AccountId, @@ -341,7 +341,7 @@ pub mod pallet { Error::::CreatorAlreadyRegistered, ); - let space_owner = T::SpacesInterface::get_space_owner(space_id)?; + let space_owner = T::SpacesProvider::get_space_owner(space_id)?; T::Currency::reserve(&space_owner, T::CreatorRegistrationDeposit::get())?; RegisteredCreators::::insert(space_id, CreatorInfo::new(space_owner.clone())); diff --git a/pallets/creator-staking/src/tests/mock.rs b/pallets/creator-staking/src/tests/mock.rs index 32b4c26a..c36fae03 100644 --- a/pallets/creator-staking/src/tests/mock.rs +++ b/pallets/creator-staking/src/tests/mock.rs @@ -126,7 +126,7 @@ impl pallet_timestamp::Config for TestRuntime { use pallet_permissions::default_permissions::DefaultSpacePermissions; use pallet_permissions::SpacePermissionsInfoOf; use subsocial_support::{Content, SpaceId}; -use subsocial_support::traits::{SpacePermissionsProvider, SpacesInterface}; +use subsocial_support::traits::{SpacePermissionsProvider, SpacesProvider}; use crate::tests::tests::Rewards; impl pallet_permissions::Config for TestRuntime { @@ -158,7 +158,7 @@ mock! { fn ensure_space_owner(id: SpaceId, account: &AccountId) -> DispatchResult; } - impl SpacesInterface for Spaces { + impl SpacesProvider for Spaces { fn get_space_owner(_space_id: SpaceId) -> Result; fn update_space_owner(_space_id: SpaceId, _new_owner: AccountId) -> DispatchResult; @@ -184,7 +184,7 @@ impl pallet_creator_staking::Config for TestRuntime { type PalletId = CreatorStakingPalletId; type BlockPerEra = BlockPerEra; type Currency = Balances; - type SpacesInterface = MockSpaces; + type SpacesProvider = MockSpaces; type SpacePermissionsProvider = MockSpaces; type CreatorRegistrationDeposit = CreatorRegistrationDeposit; type MinimumTotalStake = MinimumTotalStake; diff --git a/pallets/domains/src/lib.rs b/pallets/domains/src/lib.rs index 1b9eae48..5ea4bab6 100644 --- a/pallets/domains/src/lib.rs +++ b/pallets/domains/src/lib.rs @@ -641,7 +641,7 @@ pub mod pallet { /// Try to get domain meta by its custom and top-level domain names. /// This function pre-validates the passed argument. - pub fn require_domain_from_ref(domain: &[u8]) -> Result, DispatchError> { + pub fn require_domain_by_ref(domain: &[u8]) -> Result, DispatchError> { ensure!(domain.len() <= T::MaxDomainLength::get() as usize, Error::::DomainIsTooLong); let domain_lc = Self::lower_domain_then_bound(domain); @@ -738,17 +738,17 @@ pub mod pallet { type DomainLength = T::MaxDomainLength; fn get_domain_owner(domain: &[u8]) -> Result { - let meta = Self::require_domain_from_ref(domain)?; + let meta = Self::require_domain_by_ref(domain)?; Ok(meta.owner) } fn ensure_allowed_to_update_domain(account: &T::AccountId, domain: &[u8]) -> DispatchResult { - let meta = Self::require_domain_from_ref(domain)?; + let meta = Self::require_domain_by_ref(domain)?; Self::ensure_allowed_to_update_domain(&meta, account) } fn update_domain_owner(domain: &[u8], new_owner: &T::AccountId) -> DispatchResult { - let meta = Self::require_domain_from_ref(domain)?; + let meta = Self::require_domain_by_ref(domain)?; let domain_lc = Self::lower_domain_then_bound(domain); Self::ensure_domains_limit_not_reached(&new_owner)?; diff --git a/pallets/posts/src/functions.rs b/pallets/posts/src/functions.rs index 8ffe52cc..90561b83 100644 --- a/pallets/posts/src/functions.rs +++ b/pallets/posts/src/functions.rs @@ -144,6 +144,11 @@ impl Pallet { post: &Post, space: &Space, ) -> DispatchResult { + ensure!( + T::IsAccountBlocked::is_allowed_account(editor.clone(), space.id), + ModerationError::AccountIsBlocked + ); + let is_owner = post.is_owner(editor); let is_comment = post.is_comment(); diff --git a/pallets/posts/src/lib.rs b/pallets/posts/src/lib.rs index c9e23ee6..8d1bb2be 100644 --- a/pallets/posts/src/lib.rs +++ b/pallets/posts/src/lib.rs @@ -275,10 +275,6 @@ pub mod pallet { let space_opt = &post.try_get_space(); if let Some(space) = space_opt { - ensure!( - T::IsAccountBlocked::is_allowed_account(editor.clone(), space.id), - ModerationError::AccountIsBlocked - ); Self::ensure_account_can_update_post(&editor, &post, space)?; } diff --git a/pallets/posts/tests/src/mock.rs b/pallets/posts/tests/src/mock.rs index de59e1fd..2b38aa62 100644 --- a/pallets/posts/tests/src/mock.rs +++ b/pallets/posts/tests/src/mock.rs @@ -130,7 +130,7 @@ impl pallet_roles::Config for Test { impl pallet_profiles::Config for Test { type RuntimeEvent = RuntimeEvent; type SpacePermissionsProvider = Spaces; - type SpacesInterface = Spaces; + type SpacesProvider = Spaces; type WeightInfo = (); } @@ -177,7 +177,7 @@ impl DomainsProvider for MockEmptyDomainsProvider { impl pallet_ownership::Config for Test { type RuntimeEvent = RuntimeEvent; type ProfileManager = Profiles; - type SpacesInterface = Spaces; + type SpacesProvider = Spaces; type SpacePermissionsProvider = Spaces; type CreatorStakingProvider = (); type DomainsProvider = MockEmptyDomainsProvider; diff --git a/pallets/posts/tests/src/tests_utils.rs b/pallets/posts/tests/src/tests_utils.rs index 16b60bf9..326ddc45 100644 --- a/pallets/posts/tests/src/tests_utils.rs +++ b/pallets/posts/tests/src/tests_utils.rs @@ -13,7 +13,7 @@ use std::{ use frame_support::{assert_ok, pallet_prelude::*}; use sp_core::storage::Storage; use sp_io::TestExternalities; -use pallet_ownership::EntityWithOwnership; +use pallet_ownership::OwnableEntity; use pallet_permissions::{SpacePermission as SP, SpacePermission, SpacePermissions}; use pallet_posts::{Comment, PostExtension, PostUpdate}; @@ -489,7 +489,7 @@ pub(crate) fn _transfer_space_ownership( ) -> DispatchResult { Ownership::transfer_ownership( origin.unwrap_or_else(|| RuntimeOrigin::signed(ACCOUNT1)), - space_id.map_or(EntityWithOwnership::Space(SPACE1), |id| EntityWithOwnership::Space(id)), + space_id.map_or(OwnableEntity::Space(SPACE1), |id| OwnableEntity::Space(id)), transfer_to.unwrap_or(ACCOUNT2), ) } diff --git a/pallets/profiles/src/benchmarking.rs b/pallets/profiles/src/benchmarking.rs index f94205df..f3b47edf 100644 --- a/pallets/profiles/src/benchmarking.rs +++ b/pallets/profiles/src/benchmarking.rs @@ -10,13 +10,13 @@ use crate::Pallet as Profiles; use frame_benchmarking::{benchmarks, whitelisted_caller}; use frame_support::dispatch::DispatchError; use frame_system::RawOrigin; -use subsocial_support::{traits::SpacesInterface, Content, SpaceId}; +use subsocial_support::{traits::SpacesProvider, Content, SpaceId}; fn create_space( owner: &T::AccountId, content: Content, ) -> Result { - let space_id = T::SpacesInterface::create_space(owner, content)?; + let space_id = T::SpacesProvider::create_space(owner, content)?; Ok(space_id) } diff --git a/pallets/profiles/src/lib.rs b/pallets/profiles/src/lib.rs index b7d38e80..664785e3 100644 --- a/pallets/profiles/src/lib.rs +++ b/pallets/profiles/src/lib.rs @@ -29,7 +29,7 @@ pub mod pallet { use pallet_permissions::SpacePermissions; use subsocial_support::{ - traits::{ProfileManager, SpacePermissionsProvider, SpacesInterface}, + traits::{ProfileManager, SpacePermissionsProvider, SpacesProvider}, Content, SpaceId, SpacePermissionsInfo, }; @@ -46,7 +46,7 @@ pub mod pallet { SpacePermissionsInfoOf, >; - type SpacesInterface: SpacesInterface; + type SpacesProvider: SpacesProvider; type WeightInfo: WeightInfo; } @@ -104,7 +104,7 @@ pub mod pallet { pub fn create_space_as_profile(origin: OriginFor, content: Content) -> DispatchResult { let sender = ensure_signed(origin)?; - let space_id = T::SpacesInterface::create_space(&sender, content)?; + let space_id = T::SpacesProvider::create_space(&sender, content)?; Self::do_set_profile(&sender, space_id)?; diff --git a/pallets/profiles/src/mock.rs b/pallets/profiles/src/mock.rs index a426e0cb..3c0ffc27 100644 --- a/pallets/profiles/src/mock.rs +++ b/pallets/profiles/src/mock.rs @@ -19,7 +19,7 @@ use sp_runtime::{ }; use sp_std::sync::{Mutex, MutexGuard}; use subsocial_support::{ - traits::{SpacePermissionsProvider, SpacesInterface}, + traits::{SpacePermissionsProvider, SpacesProvider}, Content, SpaceId, }; @@ -86,7 +86,7 @@ mock! { fn ensure_space_owner(id: SpaceId, account: &AccountId) -> DispatchResult; } - impl SpacesInterface for Spaces { + impl SpacesProvider for Spaces { fn get_space_owner(_space_id: SpaceId) -> Result; fn update_space_owner(_space_id: SpaceId, _new_owner: AccountId) -> DispatchResult; @@ -98,7 +98,7 @@ mock! { impl pallet_profiles::Config for Test { type RuntimeEvent = RuntimeEvent; type SpacePermissionsProvider = MockSpaces; - type SpacesInterface = MockSpaces; + type SpacesProvider = MockSpaces; type WeightInfo = (); } diff --git a/pallets/roles/src/benchmarking.rs b/pallets/roles/src/benchmarking.rs index 1ab88d6c..739da384 100644 --- a/pallets/roles/src/benchmarking.rs +++ b/pallets/roles/src/benchmarking.rs @@ -8,7 +8,7 @@ #![cfg(feature = "runtime-benchmarks")] -// FIXME: refactor once SpacesInterface is added. +// FIXME: refactor once SpacesProvider is added. use super::*; use frame_benchmarking::{account, benchmarks}; diff --git a/pallets/space-ownership/src/benchmarking.rs b/pallets/space-ownership/src/benchmarking.rs index 5eef07a1..a238fb8b 100644 --- a/pallets/space-ownership/src/benchmarking.rs +++ b/pallets/space-ownership/src/benchmarking.rs @@ -14,7 +14,7 @@ use frame_system::RawOrigin; use sp_runtime::traits::Bounded; use subsocial_support::{ - traits::{DomainsProvider, PostsProvider, SpacesInterface}, + traits::{DomainsProvider, PostsProvider, SpacesProvider}, Content, }; @@ -33,25 +33,25 @@ fn grab_accounts() -> (T::AccountId, T::AccountId) { fn create_dummy_space( owner: &T::AccountId, -) -> Result, DispatchError> { - let space_id = T::SpacesInterface::create_space(owner, Content::None)?; - Ok(EntityWithOwnership::Space(space_id)) +) -> Result, DispatchError> { + let space_id = T::SpacesProvider::create_space(owner, Content::None)?; + Ok(OwnableEntity::Space(space_id)) } fn create_dummy_post( owner: &T::AccountId, -) -> Result, DispatchError> { - let space_id = T::SpacesInterface::create_space(owner, Content::None)?; +) -> Result, DispatchError> { + let space_id = T::SpacesProvider::create_space(owner, Content::None)?; let post_id = T::PostsProvider::create_post(owner, space_id, Content::None)?; - Ok(EntityWithOwnership::Post(post_id)) + Ok(OwnableEntity::Post(post_id)) } fn create_dummy_domain( owner: &T::AccountId, -) -> Result, DispatchError> { +) -> Result, DispatchError> { let domain = T::DomainsProvider::register_domain(owner, "dappforce.sub".as_bytes())?; let domain_bounded = domain.try_into().unwrap(); - Ok(EntityWithOwnership::Domain(domain_bounded)) + Ok(OwnableEntity::Domain(domain_bounded)) } benchmarks! { diff --git a/pallets/space-ownership/src/lib.rs b/pallets/space-ownership/src/lib.rs index b05faf0a..a41dfbf1 100644 --- a/pallets/space-ownership/src/lib.rs +++ b/pallets/space-ownership/src/lib.rs @@ -1,10 +1,8 @@ -#![cfg_attr(not(feature = "std"), no_std)] -// Copyright (C) DAPPFORCE PTE. LTD. -// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0. -// -// Full notice is available at https://github.com/dappforce/subsocial-parachain/blob/main/COPYRIGHT -// Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE +//! # Ownership Module +//! +//! This module allows the transfer of ownership of entities such as spaces, posts, and domains. +#![cfg_attr(not(feature = "std"), no_std)] use frame_system::ensure_signed; use sp_std::prelude::*; @@ -24,14 +22,14 @@ pub mod pallet { use frame_system::pallet_prelude::*; use pallet_permissions::SpacePermissions; - use subsocial_support::{PostId, SpaceId, SpacePermissionsInfo, traits::{CreatorStakingProvider, DomainsProvider, ProfileManager, SpacesInterface, PostsProvider, SpacePermissionsProvider}}; + use subsocial_support::{PostId, SpaceId, SpacePermissionsInfo, traits::{CreatorStakingProvider, DomainsProvider, ProfileManager, SpacesProvider, PostsProvider, SpacePermissionsProvider}}; pub(crate) type DomainLengthOf = <::DomainsProvider as DomainsProvider<::AccountId>>::DomainLength; #[derive(Encode, Decode, Clone, Eq, PartialEq, RuntimeDebugNoBound, TypeInfo, MaxEncodedLen)] #[scale_info(skip_type_params(T))] - pub enum EntityWithOwnership { + pub enum OwnableEntity { Space(SpaceId), Post(PostId), Domain(BoundedVec>), @@ -46,7 +44,7 @@ pub mod pallet { type ProfileManager: ProfileManager; - type SpacesInterface: SpacesInterface; + type SpacesProvider: SpacesProvider; type SpacePermissionsProvider: SpacePermissionsProvider>; @@ -77,7 +75,7 @@ pub mod pallet { /// There is no pending ownership transfer for a given entity. NoPendingTransfer, /// Account is not allowed to accept ownership transfer. - NotAllowedToAcceptOwnershipTransfer, + CurrentOwnerCannotAcceptOwnershipTransfer, /// Account is not allowed to reject ownership transfer. NotAllowedToRejectOwnershipTransfer, } @@ -85,23 +83,23 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn pending_ownership_transfer)] pub type PendingOwnershipTransfers = - StorageMap<_, Twox64Concat, EntityWithOwnership, T::AccountId>; + StorageMap<_, Twox64Concat, OwnableEntity, T::AccountId>; #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event { OwnershipTransferCreated { current_owner: T::AccountId, - entity: EntityWithOwnership, + entity: OwnableEntity, new_owner: T::AccountId, }, OwnershipTransferAccepted { account: T::AccountId, - entity: EntityWithOwnership, + entity: OwnableEntity, }, OwnershipTransferRejected { account: T::AccountId, - entity: EntityWithOwnership, + entity: OwnableEntity, }, } @@ -110,20 +108,20 @@ pub mod pallet { #[pallet::call_index(0)] #[pallet::weight( match entity { - EntityWithOwnership::Space(_) => T::WeightInfo::transfer_space_ownership(), - EntityWithOwnership::Post(_) => T::WeightInfo::transfer_post_ownership(), - EntityWithOwnership::Domain(_) => T::WeightInfo::transfer_domain_ownership(), + OwnableEntity::Space(_) => T::WeightInfo::transfer_space_ownership(), + OwnableEntity::Post(_) => T::WeightInfo::transfer_post_ownership(), + OwnableEntity::Domain(_) => T::WeightInfo::transfer_domain_ownership(), } )] pub fn transfer_ownership( origin: OriginFor, - entity: EntityWithOwnership, + entity: OwnableEntity, transfer_to: T::AccountId, ) -> DispatchResult { let who = ensure_signed(origin)?; match entity.clone() { - EntityWithOwnership::Space(space_id) => { + OwnableEntity::Space(space_id) => { ensure!(who != transfer_to, Error::::CannotTransferToCurrentOwner); T::SpacePermissionsProvider::ensure_space_owner(space_id, &who)?; @@ -135,9 +133,9 @@ pub mod pallet { Self::ensure_not_active_creator(space_id)?; } - EntityWithOwnership::Post(post_id) => + OwnableEntity::Post(post_id) => T::PostsProvider::ensure_allowed_to_update_post(&who, post_id)?, - EntityWithOwnership::Domain(domain) => + OwnableEntity::Domain(domain) => T::DomainsProvider::ensure_allowed_to_update_domain(&who, &domain)?, } @@ -154,30 +152,30 @@ pub mod pallet { #[pallet::call_index(1)] #[pallet::weight( match entity { - EntityWithOwnership::Space(_) => T::WeightInfo::accept_pending_space_ownership_transfer(), - EntityWithOwnership::Post(_) => T::WeightInfo::accept_pending_post_ownership_transfer(), - EntityWithOwnership::Domain(_) => T::WeightInfo::accept_pending_domain_ownership_transfer(), + OwnableEntity::Space(_) => T::WeightInfo::accept_pending_space_ownership_transfer(), + OwnableEntity::Post(_) => T::WeightInfo::accept_pending_post_ownership_transfer(), + OwnableEntity::Domain(_) => T::WeightInfo::accept_pending_domain_ownership_transfer(), } )] - pub fn accept_pending_ownership(origin: OriginFor, entity: EntityWithOwnership) -> DispatchResult { + pub fn accept_pending_ownership(origin: OriginFor, entity: OwnableEntity) -> DispatchResult { let new_owner = ensure_signed(origin)?; let transfer_to = Self::pending_ownership_transfer(&entity).ok_or(Error::::NoPendingTransfer)?; - ensure!(new_owner == transfer_to, Error::::NotAllowedToAcceptOwnershipTransfer); + ensure!(new_owner == transfer_to, Error::::CurrentOwnerCannotAcceptOwnershipTransfer); match entity.clone() { - EntityWithOwnership::Space(space_id) => { + OwnableEntity::Space(space_id) => { let old_space_owner = Self::get_entity_owner(&entity)?; Self::ensure_not_active_creator(space_id)?; - T::SpacesInterface::update_space_owner(space_id, transfer_to.clone())?; + T::SpacesProvider::update_space_owner(space_id, transfer_to.clone())?; T::ProfileManager::unlink_space_from_profile(&old_space_owner, space_id); } - EntityWithOwnership::Post(post_id) => + OwnableEntity::Post(post_id) => T::PostsProvider::update_post_owner(post_id, &new_owner)?, - EntityWithOwnership::Domain(domain) => + OwnableEntity::Domain(domain) => T::DomainsProvider::update_domain_owner(&domain, &new_owner)?, } @@ -192,7 +190,7 @@ pub mod pallet { #[pallet::call_index(2)] #[pallet::weight(::WeightInfo::reject_pending_ownership())] - pub fn reject_pending_ownership(origin: OriginFor, entity: EntityWithOwnership) -> DispatchResult { + pub fn reject_pending_ownership(origin: OriginFor, entity: OwnableEntity) -> DispatchResult { let who = ensure_signed(origin)?; let transfer_to = @@ -221,11 +219,11 @@ pub mod pallet { Ok(()) } - fn get_entity_owner(entity: &EntityWithOwnership) -> Result { + fn get_entity_owner(entity: &OwnableEntity) -> Result { match entity { - EntityWithOwnership::Space(space_id) => T::SpacesInterface::get_space_owner(*space_id), - EntityWithOwnership::Post(post_id) => T::PostsProvider::get_post_owner(*post_id), - EntityWithOwnership::Domain(domain) => T::DomainsProvider::get_domain_owner(domain), + OwnableEntity::Space(space_id) => T::SpacesProvider::get_space_owner(*space_id), + OwnableEntity::Post(post_id) => T::PostsProvider::get_post_owner(*post_id), + OwnableEntity::Domain(domain) => T::DomainsProvider::get_domain_owner(domain), } } } diff --git a/pallets/space-ownership/src/migration.rs b/pallets/space-ownership/src/migration.rs index cc308d70..93a4f544 100644 --- a/pallets/space-ownership/src/migration.rs +++ b/pallets/space-ownership/src/migration.rs @@ -68,7 +68,7 @@ pub mod v1 { for (space_id, account) in PendingSpaceOwner::::drain() { PendingOwnershipTransfers::::insert( - EntityWithOwnership::Space(space_id), + OwnableEntity::Space(space_id), account, ); migrated.saturating_inc(); diff --git a/pallets/space-ownership/tests/src/mock.rs b/pallets/space-ownership/tests/src/mock.rs index 8b96428e..8d051380 100644 --- a/pallets/space-ownership/tests/src/mock.rs +++ b/pallets/space-ownership/tests/src/mock.rs @@ -151,7 +151,7 @@ impl pallet_roles::Config for Test { impl pallet_profiles::Config for Test { type RuntimeEvent = RuntimeEvent; type SpacePermissionsProvider = Spaces; - type SpacesInterface = Spaces; + type SpacesProvider = Spaces; type WeightInfo = (); } @@ -205,7 +205,7 @@ impl pallet_domains::Config for Test { impl pallet_ownership::Config for Test { type RuntimeEvent = RuntimeEvent; type ProfileManager = Profiles; - type SpacesInterface = Spaces; + type SpacesProvider = Spaces; type SpacePermissionsProvider = Spaces; type CreatorStakingProvider = MockCreatorStaking; type DomainsProvider = Domains; diff --git a/pallets/space-ownership/tests/src/tests.rs b/pallets/space-ownership/tests/src/tests.rs index 155b69c6..6ecddc35 100644 --- a/pallets/space-ownership/tests/src/tests.rs +++ b/pallets/space-ownership/tests/src/tests.rs @@ -6,7 +6,7 @@ use frame_support::{assert_noop, assert_ok}; -use pallet_ownership::{EntityWithOwnership, Error as OwnershipError, Event as OwnershipEvent}; +use pallet_ownership::{OwnableEntity, Error as OwnershipError, Event as OwnershipEvent}; use crate::{mock::*, tests_utils::*}; @@ -80,7 +80,7 @@ fn transfer_ownership_should_not_allow_transfer_to_current_owner() { assert_noop!( Ownership::transfer_ownership( RuntimeOrigin::signed(ACCOUNT1), - EntityWithOwnership::Space(SPACE1), + OwnableEntity::Space(SPACE1), ACCOUNT1 ), OwnershipError::::CannotTransferToCurrentOwner @@ -101,7 +101,7 @@ fn transfer_ownership_should_not_allow_active_creator_to_transfer_space_ownershi assert_noop!( Ownership::transfer_ownership( RuntimeOrigin::signed(ACCOUNT1), - EntityWithOwnership::Space(SPACE1), + OwnableEntity::Space(SPACE1), ACCOUNT2 ), OwnershipError::::ActiveCreatorCannotTransferOwnership @@ -116,7 +116,7 @@ fn accept_pending_ownership_should_fail_if_no_pending_transfer() { assert_noop!( Ownership::accept_pending_ownership( RuntimeOrigin::signed(ACCOUNT2), - EntityWithOwnership::Space(SPACE1) + OwnableEntity::Space(SPACE1) ), OwnershipError::::NoPendingTransfer ); @@ -136,15 +136,15 @@ fn accept_pending_ownership_should_not_allow_non_target_to_accept() { assert_noop!( Ownership::accept_pending_ownership( RuntimeOrigin::signed(ACCOUNT3), - EntityWithOwnership::Space(SPACE1) + OwnableEntity::Space(SPACE1) ), - OwnershipError::::NotAllowedToAcceptOwnershipTransfer + OwnershipError::::CurrentOwnerCannotAcceptOwnershipTransfer ); assert_ok!( Ownership::accept_pending_ownership( RuntimeOrigin::signed(ACCOUNT2), - EntityWithOwnership::Space(SPACE1) + OwnableEntity::Space(SPACE1) ) ); }); @@ -157,7 +157,7 @@ fn reject_pending_ownership_should_fail_if_no_pending_transfer() { assert_noop!( Ownership::reject_pending_ownership( RuntimeOrigin::signed(ACCOUNT1), - EntityWithOwnership::Space(SPACE1) + OwnableEntity::Space(SPACE1) ), OwnershipError::::NoPendingTransfer ); @@ -171,7 +171,7 @@ fn reject_pending_ownership_should_not_allow_ineligible_account_to_reject() { assert_noop!( Ownership::reject_pending_ownership( RuntimeOrigin::signed(ACCOUNT3), - EntityWithOwnership::Space(SPACE1) + OwnableEntity::Space(SPACE1) ), OwnershipError::::NotAllowedToRejectOwnershipTransfer ); @@ -180,7 +180,7 @@ fn reject_pending_ownership_should_not_allow_ineligible_account_to_reject() { assert_ok!( Ownership::reject_pending_ownership( RuntimeOrigin::signed(ACCOUNT1), - EntityWithOwnership::Space(SPACE1) + OwnableEntity::Space(SPACE1) ) ); }); diff --git a/pallets/space-ownership/tests/src/tests_utils.rs b/pallets/space-ownership/tests/src/tests_utils.rs index d4b67590..0092fd8a 100644 --- a/pallets/space-ownership/tests/src/tests_utils.rs +++ b/pallets/space-ownership/tests/src/tests_utils.rs @@ -7,7 +7,7 @@ use frame_support::{assert_ok, pallet_prelude::*}; use sp_core::storage::Storage; use sp_io::TestExternalities; -use pallet_ownership::{EntityWithOwnership, Event as OwnershipEvent}; +use pallet_ownership::{OwnableEntity, Event as OwnershipEvent}; use pallet_posts::PostExtension; use pallet_spaces::*; @@ -100,16 +100,16 @@ pub(crate) fn default_domain() -> BoundedVec { "dappforce.sub".as_bytes().to_vec().try_into().unwrap() } -pub(crate) fn default_domain_entity() -> EntityWithOwnership { - EntityWithOwnership::Domain(default_domain()) +pub(crate) fn default_domain_entity() -> OwnableEntity { + OwnableEntity::Domain(default_domain()) } -pub(crate) const fn default_space_entity() -> EntityWithOwnership { - EntityWithOwnership::Space(SPACE1) +pub(crate) const fn default_space_entity() -> OwnableEntity { + OwnableEntity::Space(SPACE1) } -pub(crate) const fn default_post_entity() -> EntityWithOwnership { - EntityWithOwnership::Post(POST1) +pub(crate) const fn default_post_entity() -> OwnableEntity { + OwnableEntity::Post(POST1) } // Extrinsics @@ -145,7 +145,7 @@ pub(crate) fn _create_default_domain() -> DispatchResult { pub(crate) fn _transfer_ownership( account: AccountId, - entity: EntityWithOwnership, + entity: OwnableEntity, transfer_to: AccountId, ) -> DispatchResult { Ownership::transfer_ownership( @@ -157,7 +157,7 @@ pub(crate) fn _transfer_ownership( pub(crate) fn _accept_pending_ownership( account: AccountId, - entity: EntityWithOwnership, + entity: OwnableEntity, ) -> DispatchResult { Ownership::accept_pending_ownership( RuntimeOrigin::signed(account), @@ -167,7 +167,7 @@ pub(crate) fn _accept_pending_ownership( pub(crate) fn _reject_pending_ownership( account: AccountId, - entity: EntityWithOwnership, + entity: OwnableEntity, ) -> DispatchResult { Ownership::reject_pending_ownership( RuntimeOrigin::signed(account), @@ -184,21 +184,21 @@ pub(crate) fn assert_create_transfers_ok() { // Mock a transfer from account 1 to account 2 for a space entity assert_ok!(Ownership::transfer_ownership( RuntimeOrigin::signed(ACCOUNT1), - EntityWithOwnership::Space(SPACE1), + OwnableEntity::Space(SPACE1), ACCOUNT2 )); // Mock a transfer from account 1 to account 2 for a post entity assert_ok!(Ownership::transfer_ownership( RuntimeOrigin::signed(ACCOUNT1), - EntityWithOwnership::Post(POST1), + OwnableEntity::Post(POST1), ACCOUNT2 )); // Mock a transfer from account 1 to account 2 for a domain entity assert_ok!(Ownership::transfer_ownership( RuntimeOrigin::signed(ACCOUNT1), - EntityWithOwnership::Domain(default_domain()), + OwnableEntity::Domain(default_domain()), ACCOUNT2 )); diff --git a/pallets/spaces/src/lib.rs b/pallets/spaces/src/lib.rs index ba1a1228..f35d8ac2 100644 --- a/pallets/spaces/src/lib.rs +++ b/pallets/spaces/src/lib.rs @@ -49,7 +49,7 @@ pub mod pallet { }; use subsocial_support::{ ensure_content_is_valid, remove_from_bounded_vec, - traits::{IsAccountBlocked, IsContentBlocked, SpacePermissionsProvider, SpacesInterface}, + traits::{IsAccountBlocked, IsContentBlocked, SpacePermissionsProvider, SpacesProvider}, ModerationError, SpacePermissionsInfo, WhoAndWhen, WhoAndWhenOf, }; use types::*; @@ -426,7 +426,7 @@ pub mod pallet { } } - impl SpacesInterface for Pallet { + impl SpacesProvider for Pallet { fn get_space_owner(space_id: SpaceId) -> Result { let space = Pallet::::require_space(space_id)?; Ok(space.owner) diff --git a/pallets/spaces/tests/src/mock.rs b/pallets/spaces/tests/src/mock.rs index 9aad34e5..b441b844 100644 --- a/pallets/spaces/tests/src/mock.rs +++ b/pallets/spaces/tests/src/mock.rs @@ -130,7 +130,7 @@ impl pallet_roles::Config for Test { impl pallet_profiles::Config for Test { type RuntimeEvent = RuntimeEvent; type SpacePermissionsProvider = Spaces; - type SpacesInterface = Spaces; + type SpacesProvider = Spaces; type WeightInfo = (); } diff --git a/pallets/support/src/traits.rs b/pallets/support/src/traits.rs index fd033b5b..99477fde 100644 --- a/pallets/support/src/traits.rs +++ b/pallets/support/src/traits.rs @@ -6,7 +6,7 @@ pub use common::{ CreatorStakingProvider, DomainsProvider, PostFollowsProvider, PostsProvider, ProfileManager, - SpaceFollowsProvider, SpacePermissionsProvider, SpacesInterface, + SpaceFollowsProvider, SpacePermissionsProvider, SpacesProvider, }; pub use moderation::{IsAccountBlocked, IsContentBlocked, IsPostBlocked, IsSpaceBlocked}; diff --git a/pallets/support/src/traits/common.rs b/pallets/support/src/traits/common.rs index 45cd7ac3..d18abef8 100644 --- a/pallets/support/src/traits/common.rs +++ b/pallets/support/src/traits/common.rs @@ -32,8 +32,7 @@ pub trait ProfileManager { fn unlink_space_from_profile(account: &AccountId, space_id: SpaceId); } -// TODO: rename to `SpacesProvider` -pub trait SpacesInterface { +pub trait SpacesProvider { fn get_space_owner(space_id: SpaceId) -> Result; diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 4d6e1ff2..13b8bf3a 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -739,7 +739,7 @@ impl pallet_reactions::Config for Runtime { impl pallet_profiles::Config for Runtime { type RuntimeEvent = RuntimeEvent; type SpacePermissionsProvider = Spaces; - type SpacesInterface = Spaces; + type SpacesProvider = Spaces; type WeightInfo = pallet_profiles::weights::SubstrateWeight; } @@ -779,7 +779,7 @@ impl pallet_spaces::Config for Runtime { impl pallet_ownership::Config for Runtime { type RuntimeEvent = RuntimeEvent; type ProfileManager = Profiles; - type SpacesInterface = Spaces; + type SpacesProvider = Spaces; type SpacePermissionsProvider = Spaces; type CreatorStakingProvider = CreatorStaking; type DomainsProvider = Domains; @@ -841,7 +841,7 @@ impl pallet_creator_staking::Config for Runtime { type PalletId = CreatorStakingPalletId; type BlockPerEra = BlockPerEra; type Currency = Balances; - type SpacesInterface = Spaces; + type SpacesProvider = Spaces; type SpacePermissionsProvider = Spaces; type CreatorRegistrationDeposit = CreatorRegistrationDeposit; type MinimumTotalStake = MinimumTotalStake; From 056cb68bfc9e1d6a40740ff39a6ec51c9bf9c327 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 22 Mar 2024 11:34:48 +0000 Subject: [PATCH 19/25] Add missing license headers --- pallets/space-ownership/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pallets/space-ownership/src/lib.rs b/pallets/space-ownership/src/lib.rs index a41dfbf1..f8b703de 100644 --- a/pallets/space-ownership/src/lib.rs +++ b/pallets/space-ownership/src/lib.rs @@ -1,3 +1,9 @@ +// Copyright (C) DAPPFORCE PTE. LTD. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0. +// +// Full notice is available at https://github.com/dappforce/subsocial-parachain/blob/main/COPYRIGHT +// Full license is available at https://github.com/dappforce/subsocial-parachain/blob/main/LICENSE + //! # Ownership Module //! //! This module allows the transfer of ownership of entities such as spaces, posts, and domains. From f575b39815180beec27e70bd7e5034320f07460d Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Fri, 22 Mar 2024 13:35:20 +0200 Subject: [PATCH 20/25] Apply suggestions from code review Co-authored-by: Alex Siman --- pallets/domains/src/lib.rs | 4 ++-- pallets/space-ownership/tests/src/tests.rs | 2 +- pallets/spaces/src/lib.rs | 1 - 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/pallets/domains/src/lib.rs b/pallets/domains/src/lib.rs index 5ea4bab6..a9703b31 100644 --- a/pallets/domains/src/lib.rs +++ b/pallets/domains/src/lib.rs @@ -193,9 +193,9 @@ pub mod pallet { TooManyDomainsPerAccount, /// This domain label may contain only a-z, 0-9 and hyphen characters. DomainContainsInvalidChar, - /// This domain name length must be greater or equal the [`Config::MinDomainLength`] limit. + /// The length of this domain name must be greater than or equal to the [`Config::MinDomainLength`] limit. DomainIsTooShort, - /// This domain name length must be below or equal the [`Config::MaxDomainLength`] limit. + /// The length of this domain name must be less than or equal to the [`Config::MaxDomainLength`] limit. DomainIsTooLong, /// This domain has expired. DomainHasExpired, diff --git a/pallets/space-ownership/tests/src/tests.rs b/pallets/space-ownership/tests/src/tests.rs index 6ecddc35..1aa5058f 100644 --- a/pallets/space-ownership/tests/src/tests.rs +++ b/pallets/space-ownership/tests/src/tests.rs @@ -124,7 +124,7 @@ fn accept_pending_ownership_should_fail_if_no_pending_transfer() { } #[test] -fn accept_pending_ownership_should_not_allow_non_target_to_accept() { +fn accept_pending_ownership_should_not_allow_non_target_account_to_accept() { ExtBuilder::build_with_pending_transfers().execute_with(|| { let _m = use_static_mock(); let creator_staking_ctx = MockCreatorStaking::is_creator_active_context(); diff --git a/pallets/spaces/src/lib.rs b/pallets/spaces/src/lib.rs index f35d8ac2..64ccb7bb 100644 --- a/pallets/spaces/src/lib.rs +++ b/pallets/spaces/src/lib.rs @@ -436,7 +436,6 @@ pub mod pallet { Self::ensure_space_limit_not_reached(&new_owner)?; let space = Pallet::::require_space(space_id)?; - // TODO: reuse copy-pasted parts of code SpaceIdsByOwner::::mutate(&space.owner, |ids| { remove_from_bounded_vec(ids, space_id) }); From 26aa8594d0421100232e30b9a9567d7aa46ba335 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Wed, 27 Mar 2024 15:17:23 +0200 Subject: [PATCH 21/25] Apply suggestions from code review Co-Authored-By: Alex Siman --- .../src/tests/space_ownership.rs | 4 +- pallets/domains/src/lib.rs | 10 +-- pallets/domains/src/types.rs | 4 ++ pallets/posts/src/functions.rs | 6 +- pallets/posts/tests/src/mock.rs | 2 +- pallets/space-ownership/src/lib.rs | 65 ++++++++----------- pallets/space-ownership/tests/src/tests.rs | 2 +- pallets/spaces/src/lib.rs | 5 ++ pallets/support/src/traits/common.rs | 4 +- 9 files changed, 51 insertions(+), 51 deletions(-) diff --git a/integration-tests/src/tests/space_ownership.rs b/integration-tests/src/tests/space_ownership.rs index f696550d..4553db70 100644 --- a/integration-tests/src/tests/space_ownership.rs +++ b/integration-tests/src/tests/space_ownership.rs @@ -108,7 +108,7 @@ fn accept_pending_ownership_should_fail_if_origin_is_already_an_owner() { assert_noop!( _accept_pending_ownership(Some(RuntimeOrigin::signed(ACCOUNT1)), None), - SpaceOwnershipError::::CurrentOwnerCannotAcceptOwnershipTransfer, + SpaceOwnershipError::::NotAllowedToAcceptOwnershipTransfer, ); }); } @@ -120,7 +120,7 @@ fn accept_pending_ownership_should_fail_if_origin_is_not_equal_to_pending_accoun assert_noop!( _accept_pending_ownership(Some(RuntimeOrigin::signed(ACCOUNT3)), None), - SpaceOwnershipError::::CurrentOwnerCannotAcceptOwnershipTransfer + SpaceOwnershipError::::NotAllowedToAcceptOwnershipTransfer ); }); } diff --git a/pallets/domains/src/lib.rs b/pallets/domains/src/lib.rs index a9703b31..08b084e3 100644 --- a/pallets/domains/src/lib.rs +++ b/pallets/domains/src/lib.rs @@ -653,11 +653,10 @@ pub mod pallet { domain_meta: &DomainMeta, sender: &T::AccountId, ) -> DispatchResult { - let DomainMeta { owner, .. } = domain_meta; - // FIXME: this is hotfix, handle expired domains correctly! // ensure!(&System::::block_number() < expires_at, Error::::DomainHasExpired); - ensure!(sender == owner, Error::::NotDomainOwner); + + ensure!(domain_meta.is_owner(sender), Error::::NotDomainOwner); Ok(()) } @@ -742,9 +741,10 @@ pub mod pallet { Ok(meta.owner) } - fn ensure_allowed_to_update_domain(account: &T::AccountId, domain: &[u8]) -> DispatchResult { + fn ensure_domain_owner(account: &T::AccountId, domain: &[u8]) -> DispatchResult { let meta = Self::require_domain_by_ref(domain)?; - Self::ensure_allowed_to_update_domain(&meta, account) + ensure!(meta.is_owner(account), Error::::NotDomainOwner); + Ok(()) } fn update_domain_owner(domain: &[u8], new_owner: &T::AccountId) -> DispatchResult { diff --git a/pallets/domains/src/types.rs b/pallets/domains/src/types.rs index 7111a33c..1d778cae 100644 --- a/pallets/domains/src/types.rs +++ b/pallets/domains/src/types.rs @@ -103,6 +103,10 @@ impl DomainMeta { outer_value_deposit: Zero::zero(), } } + + pub fn is_owner(&self, account: &T::AccountId) -> bool { + self.owner == *account + } } impl From<(AccountId, Balance)> for DomainDeposit { diff --git a/pallets/posts/src/functions.rs b/pallets/posts/src/functions.rs index 90561b83..102c7346 100644 --- a/pallets/posts/src/functions.rs +++ b/pallets/posts/src/functions.rs @@ -466,10 +466,10 @@ impl PostsProvider for Pallet { Ok(post.owner) } - fn ensure_allowed_to_update_post(account: &T::AccountId, post_id: PostId) -> DispatchResult { + fn ensure_post_owner(account: &T::AccountId, post_id: PostId) -> DispatchResult { let post = Self::require_post(post_id)?; - let space = post.get_space()?; - Self::ensure_account_can_update_post(account, &post, &space) + ensure!(post.is_owner(account), Error::::NotAPostOwner); + Ok(()) } fn update_post_owner(post_id: PostId, new_owner: &T::AccountId) -> DispatchResult { diff --git a/pallets/posts/tests/src/mock.rs b/pallets/posts/tests/src/mock.rs index 2b38aa62..9392539f 100644 --- a/pallets/posts/tests/src/mock.rs +++ b/pallets/posts/tests/src/mock.rs @@ -160,7 +160,7 @@ impl DomainsProvider for MockEmptyDomainsProvider { Ok(ACCOUNT1) } - fn ensure_allowed_to_update_domain(_account: &AccountId, _domain: &[u8]) -> DispatchResult { + fn ensure_domain_owner(_account: &AccountId, _domain: &[u8]) -> DispatchResult { Ok(()) } diff --git a/pallets/space-ownership/src/lib.rs b/pallets/space-ownership/src/lib.rs index f8b703de..59e6e58b 100644 --- a/pallets/space-ownership/src/lib.rs +++ b/pallets/space-ownership/src/lib.rs @@ -81,7 +81,7 @@ pub mod pallet { /// There is no pending ownership transfer for a given entity. NoPendingTransfer, /// Account is not allowed to accept ownership transfer. - CurrentOwnerCannotAcceptOwnershipTransfer, + NotAllowedToAcceptOwnershipTransfer, /// Account is not allowed to reject ownership transfer. NotAllowedToRejectOwnershipTransfer, } @@ -122,35 +122,29 @@ pub mod pallet { pub fn transfer_ownership( origin: OriginFor, entity: OwnableEntity, - transfer_to: T::AccountId, + new_owner: T::AccountId, ) -> DispatchResult { - let who = ensure_signed(origin)?; + let current_owner = ensure_signed(origin)?; + + ensure!(current_owner != new_owner, Error::::CannotTransferToCurrentOwner); match entity.clone() { OwnableEntity::Space(space_id) => { - ensure!(who != transfer_to, Error::::CannotTransferToCurrentOwner); - - T::SpacePermissionsProvider::ensure_space_owner(space_id, &who)?; - // TODO: ensure we need this kind of moderation checks - // ensure!( - // T::IsAccountBlocked::is_allowed_account(transfer_to.clone(), space_id), - // ModerationError::AccountIsBlocked - // ); - + T::SpacePermissionsProvider::ensure_space_owner(space_id, ¤t_owner)?; Self::ensure_not_active_creator(space_id)?; } OwnableEntity::Post(post_id) => - T::PostsProvider::ensure_allowed_to_update_post(&who, post_id)?, + T::PostsProvider::ensure_post_owner(¤t_owner, post_id)?, OwnableEntity::Domain(domain) => - T::DomainsProvider::ensure_allowed_to_update_domain(&who, &domain)?, + T::DomainsProvider::ensure_domain_owner(¤t_owner, &domain)?, } - PendingOwnershipTransfers::::insert(&entity, transfer_to.clone()); + PendingOwnershipTransfers::::insert(&entity, new_owner.clone()); Self::deposit_event(Event::OwnershipTransferCreated { - current_owner: who, + current_owner, entity, - new_owner: transfer_to, + new_owner, }); Ok(()) } @@ -164,31 +158,32 @@ pub mod pallet { } )] pub fn accept_pending_ownership(origin: OriginFor, entity: OwnableEntity) -> DispatchResult { - let new_owner = ensure_signed(origin)?; + let ownership_claimant = ensure_signed(origin)?; - let transfer_to = + let pending_owner = Self::pending_ownership_transfer(&entity).ok_or(Error::::NoPendingTransfer)?; - ensure!(new_owner == transfer_to, Error::::CurrentOwnerCannotAcceptOwnershipTransfer); + ensure!(ownership_claimant == pending_owner, Error::::NotAllowedToAcceptOwnershipTransfer); match entity.clone() { OwnableEntity::Space(space_id) => { - let old_space_owner = Self::get_entity_owner(&entity)?; + let previous_space_owner = T::SpacesProvider::get_space_owner(space_id)?; Self::ensure_not_active_creator(space_id)?; - T::SpacesProvider::update_space_owner(space_id, transfer_to.clone())?; - T::ProfileManager::unlink_space_from_profile(&old_space_owner, space_id); + + T::SpacesProvider::update_space_owner(space_id, pending_owner.clone())?; + T::ProfileManager::unlink_space_from_profile(&previous_space_owner, space_id); } OwnableEntity::Post(post_id) => - T::PostsProvider::update_post_owner(post_id, &new_owner)?, + T::PostsProvider::update_post_owner(post_id, &ownership_claimant)?, OwnableEntity::Domain(domain) => - T::DomainsProvider::update_domain_owner(&domain, &new_owner)?, + T::DomainsProvider::update_domain_owner(&domain, &ownership_claimant)?, } PendingOwnershipTransfers::::remove(&entity); Self::deposit_event(Event::OwnershipTransferAccepted { - account: new_owner, + account: ownership_claimant, entity, }); Ok(()) @@ -199,12 +194,16 @@ pub mod pallet { pub fn reject_pending_ownership(origin: OriginFor, entity: OwnableEntity) -> DispatchResult { let who = ensure_signed(origin)?; - let transfer_to = + let pending_owner = Self::pending_ownership_transfer(&entity).ok_or(Error::::NoPendingTransfer)?; - let entity_owner = Self::get_entity_owner(&entity)?; + let current_owner = match entity.clone() { + OwnableEntity::Space(space_id) => T::SpacesProvider::get_space_owner(space_id), + OwnableEntity::Post(post_id) => T::PostsProvider::get_post_owner(post_id), + OwnableEntity::Domain(domain) => T::DomainsProvider::get_domain_owner(&domain), + }?; ensure!( - who == transfer_to || who == entity_owner, + who == pending_owner || who == current_owner, Error::::NotAllowedToRejectOwnershipTransfer ); @@ -224,13 +223,5 @@ pub mod pallet { Ok(()) } - - fn get_entity_owner(entity: &OwnableEntity) -> Result { - match entity { - OwnableEntity::Space(space_id) => T::SpacesProvider::get_space_owner(*space_id), - OwnableEntity::Post(post_id) => T::PostsProvider::get_post_owner(*post_id), - OwnableEntity::Domain(domain) => T::DomainsProvider::get_domain_owner(domain), - } - } } } diff --git a/pallets/space-ownership/tests/src/tests.rs b/pallets/space-ownership/tests/src/tests.rs index 1aa5058f..9209ab70 100644 --- a/pallets/space-ownership/tests/src/tests.rs +++ b/pallets/space-ownership/tests/src/tests.rs @@ -138,7 +138,7 @@ fn accept_pending_ownership_should_not_allow_non_target_account_to_accept() { RuntimeOrigin::signed(ACCOUNT3), OwnableEntity::Space(SPACE1) ), - OwnershipError::::CurrentOwnerCannotAcceptOwnershipTransfer + OwnershipError::::NotAllowedToAcceptOwnershipTransfer ); assert_ok!( diff --git a/pallets/spaces/src/lib.rs b/pallets/spaces/src/lib.rs index 64ccb7bb..6743155a 100644 --- a/pallets/spaces/src/lib.rs +++ b/pallets/spaces/src/lib.rs @@ -436,6 +436,11 @@ pub mod pallet { Self::ensure_space_limit_not_reached(&new_owner)?; let space = Pallet::::require_space(space_id)?; + ensure!( + T::IsAccountBlocked::is_allowed_account(new_owner.clone(), space_id), + ModerationError::AccountIsBlocked + ); + SpaceIdsByOwner::::mutate(&space.owner, |ids| { remove_from_bounded_vec(ids, space_id) }); diff --git a/pallets/support/src/traits/common.rs b/pallets/support/src/traits/common.rs index d18abef8..b75dd695 100644 --- a/pallets/support/src/traits/common.rs +++ b/pallets/support/src/traits/common.rs @@ -60,7 +60,7 @@ pub trait DomainsProvider { fn get_domain_owner(domain: &[u8]) -> Result; - fn ensure_allowed_to_update_domain(account: &AccountId, domain: &[u8]) -> DispatchResult; + fn ensure_domain_owner(account: &AccountId, domain: &[u8]) -> DispatchResult; fn update_domain_owner(domain: &[u8], new_owner: &AccountId) -> DispatchResult; @@ -71,7 +71,7 @@ pub trait DomainsProvider { pub trait PostsProvider { fn get_post_owner(post_id: PostId) -> Result; - fn ensure_allowed_to_update_post(account: &AccountId, post_id: PostId) -> DispatchResult; + fn ensure_post_owner(account: &AccountId, post_id: PostId) -> DispatchResult; fn update_post_owner(post_id: PostId, new_owner: &AccountId) -> DispatchResult; From 2cb2a22e8ee595d760bfda342bdf6e83248aea3c Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Wed, 27 Mar 2024 15:35:27 +0200 Subject: [PATCH 22/25] Change some functions arguments ordering --- pallets/domains/src/lib.rs | 2 +- pallets/posts/src/functions.rs | 2 +- pallets/posts/tests/src/mock.rs | 2 +- pallets/space-ownership/src/lib.rs | 14 +++++++------- pallets/support/src/traits/common.rs | 4 ++-- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/pallets/domains/src/lib.rs b/pallets/domains/src/lib.rs index 08b084e3..37eafa10 100644 --- a/pallets/domains/src/lib.rs +++ b/pallets/domains/src/lib.rs @@ -741,7 +741,7 @@ pub mod pallet { Ok(meta.owner) } - fn ensure_domain_owner(account: &T::AccountId, domain: &[u8]) -> DispatchResult { + fn ensure_domain_owner(domain: &[u8], account: &T::AccountId) -> DispatchResult { let meta = Self::require_domain_by_ref(domain)?; ensure!(meta.is_owner(account), Error::::NotDomainOwner); Ok(()) diff --git a/pallets/posts/src/functions.rs b/pallets/posts/src/functions.rs index 102c7346..3e395f3e 100644 --- a/pallets/posts/src/functions.rs +++ b/pallets/posts/src/functions.rs @@ -466,7 +466,7 @@ impl PostsProvider for Pallet { Ok(post.owner) } - fn ensure_post_owner(account: &T::AccountId, post_id: PostId) -> DispatchResult { + fn ensure_post_owner(post_id: PostId, account: &T::AccountId) -> DispatchResult { let post = Self::require_post(post_id)?; ensure!(post.is_owner(account), Error::::NotAPostOwner); Ok(()) diff --git a/pallets/posts/tests/src/mock.rs b/pallets/posts/tests/src/mock.rs index 9392539f..d3dbce78 100644 --- a/pallets/posts/tests/src/mock.rs +++ b/pallets/posts/tests/src/mock.rs @@ -160,7 +160,7 @@ impl DomainsProvider for MockEmptyDomainsProvider { Ok(ACCOUNT1) } - fn ensure_domain_owner(_account: &AccountId, _domain: &[u8]) -> DispatchResult { + fn ensure_domain_owner(_domain: &[u8], _account: &AccountId) -> DispatchResult { Ok(()) } diff --git a/pallets/space-ownership/src/lib.rs b/pallets/space-ownership/src/lib.rs index 59e6e58b..33616989 100644 --- a/pallets/space-ownership/src/lib.rs +++ b/pallets/space-ownership/src/lib.rs @@ -134,9 +134,9 @@ pub mod pallet { Self::ensure_not_active_creator(space_id)?; } OwnableEntity::Post(post_id) => - T::PostsProvider::ensure_post_owner(¤t_owner, post_id)?, + T::PostsProvider::ensure_post_owner(post_id, ¤t_owner)?, OwnableEntity::Domain(domain) => - T::DomainsProvider::ensure_domain_owner(¤t_owner, &domain)?, + T::DomainsProvider::ensure_domain_owner(&domain, ¤t_owner)?, } PendingOwnershipTransfers::::insert(&entity, new_owner.clone()); @@ -167,23 +167,23 @@ pub mod pallet { match entity.clone() { OwnableEntity::Space(space_id) => { - let previous_space_owner = T::SpacesProvider::get_space_owner(space_id)?; + let previous_owner = T::SpacesProvider::get_space_owner(space_id)?; Self::ensure_not_active_creator(space_id)?; T::SpacesProvider::update_space_owner(space_id, pending_owner.clone())?; - T::ProfileManager::unlink_space_from_profile(&previous_space_owner, space_id); + T::ProfileManager::unlink_space_from_profile(&previous_owner, space_id); } OwnableEntity::Post(post_id) => - T::PostsProvider::update_post_owner(post_id, &ownership_claimant)?, + T::PostsProvider::update_post_owner(post_id, &pending_owner)?, OwnableEntity::Domain(domain) => - T::DomainsProvider::update_domain_owner(&domain, &ownership_claimant)?, + T::DomainsProvider::update_domain_owner(&domain, &pending_owner)?, } PendingOwnershipTransfers::::remove(&entity); Self::deposit_event(Event::OwnershipTransferAccepted { - account: ownership_claimant, + account: pending_owner, entity, }); Ok(()) diff --git a/pallets/support/src/traits/common.rs b/pallets/support/src/traits/common.rs index b75dd695..e6dfcd92 100644 --- a/pallets/support/src/traits/common.rs +++ b/pallets/support/src/traits/common.rs @@ -60,7 +60,7 @@ pub trait DomainsProvider { fn get_domain_owner(domain: &[u8]) -> Result; - fn ensure_domain_owner(account: &AccountId, domain: &[u8]) -> DispatchResult; + fn ensure_domain_owner(domain: &[u8], account: &AccountId) -> DispatchResult; fn update_domain_owner(domain: &[u8], new_owner: &AccountId) -> DispatchResult; @@ -71,7 +71,7 @@ pub trait DomainsProvider { pub trait PostsProvider { fn get_post_owner(post_id: PostId) -> Result; - fn ensure_post_owner(account: &AccountId, post_id: PostId) -> DispatchResult; + fn ensure_post_owner(post_id: PostId, account: &AccountId) -> DispatchResult; fn update_post_owner(post_id: PostId, new_owner: &AccountId) -> DispatchResult; From 6166d7f0fc75b76a4b5e95a2bff26c1128bbc390 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Wed, 27 Mar 2024 16:25:09 +0200 Subject: [PATCH 23/25] Apply final suggestions after code review Co-Authored-By: Alex Siman --- pallets/creator-staking/src/tests/mock.rs | 2 +- pallets/domains/src/lib.rs | 24 +++++++++++++++++++---- pallets/posts/src/functions.rs | 7 ++++++- pallets/posts/tests/src/mock.rs | 4 ++-- pallets/profiles/src/mock.rs | 2 +- pallets/space-ownership/src/lib.rs | 12 ++++++------ pallets/spaces/src/lib.rs | 8 ++++++-- pallets/support/src/traits/common.rs | 8 ++++---- 8 files changed, 46 insertions(+), 21 deletions(-) diff --git a/pallets/creator-staking/src/tests/mock.rs b/pallets/creator-staking/src/tests/mock.rs index c36fae03..546f28a1 100644 --- a/pallets/creator-staking/src/tests/mock.rs +++ b/pallets/creator-staking/src/tests/mock.rs @@ -161,7 +161,7 @@ mock! { impl SpacesProvider for Spaces { fn get_space_owner(_space_id: SpaceId) -> Result; - fn update_space_owner(_space_id: SpaceId, _new_owner: AccountId) -> DispatchResult; + fn do_update_space_owner(_space_id: SpaceId, _new_owner: AccountId) -> DispatchResult; fn create_space(_owner: &AccountId, _content: Content) -> Result; } diff --git a/pallets/domains/src/lib.rs b/pallets/domains/src/lib.rs index 37eafa10..1ded0e0e 100644 --- a/pallets/domains/src/lib.rs +++ b/pallets/domains/src/lib.rs @@ -552,6 +552,15 @@ pub mod pallet { ); Ok(()) } + + /// Throws an error if domain length exceeds the `DomainName` BoundedVec size limit. + fn ensure_valid_domain_length(domain: &[u8]) -> DispatchResult { + ensure!( + domain.len() <= T::MaxDomainLength::get() as usize, + Error::::DomainIsTooLong, + ); + Ok(()) + } /// Throws an error if domain contains invalid character. fn ensure_domain_contains_valid_chars(domain: &[u8]) -> DispatchResult { @@ -642,7 +651,10 @@ pub mod pallet { /// Try to get domain meta by its custom and top-level domain names. /// This function pre-validates the passed argument. pub fn require_domain_by_ref(domain: &[u8]) -> Result, DispatchError> { - ensure!(domain.len() <= T::MaxDomainLength::get() as usize, Error::::DomainIsTooLong); + // Because of BoundedVec implementation speicifics, we need to + // check that the domain length does not exceed the BoundedVec size + // before converting from &[u8] to DomainName in `lower_domain_then_bound`, + Self::ensure_valid_domain_length(domain)?; let domain_lc = Self::lower_domain_then_bound(domain); Self::require_domain(domain_lc) @@ -734,7 +746,7 @@ pub mod pallet { } impl DomainsProvider for Pallet { - type DomainLength = T::MaxDomainLength; + type MaxDomainLength = T::MaxDomainLength; fn get_domain_owner(domain: &[u8]) -> Result { let meta = Self::require_domain_by_ref(domain)?; @@ -747,8 +759,12 @@ pub mod pallet { Ok(()) } - fn update_domain_owner(domain: &[u8], new_owner: &T::AccountId) -> DispatchResult { + fn do_update_domain_owner(domain: &[u8], new_owner: &T::AccountId) -> DispatchResult { let meta = Self::require_domain_by_ref(domain)?; + if meta.is_owner(new_owner) { + return Ok(()); + } + let domain_lc = Self::lower_domain_then_bound(domain); Self::ensure_domains_limit_not_reached(&new_owner)?; @@ -778,7 +794,7 @@ pub mod pallet { #[cfg(feature = "runtime-benchmarks")] fn register_domain(owner: &T::AccountId, domain: &[u8]) -> Result, DispatchError> { - ensure!(domain.len() <= T::MaxDomainLength::get() as usize, Error::::DomainIsTooLong); + Self::ensure_valid_domain_length(domain)?; let domain_lc = Self::lower_domain_then_bound(domain); Self::do_register_domain( diff --git a/pallets/posts/src/functions.rs b/pallets/posts/src/functions.rs index 3e395f3e..fc5cbca1 100644 --- a/pallets/posts/src/functions.rs +++ b/pallets/posts/src/functions.rs @@ -472,7 +472,12 @@ impl PostsProvider for Pallet { Ok(()) } - fn update_post_owner(post_id: PostId, new_owner: &T::AccountId) -> DispatchResult { + fn do_update_post_owner(post_id: PostId, new_owner: &T::AccountId) -> DispatchResult { + let post = Self::require_post(post_id)?; + if post.is_owner(new_owner) { + return Ok(()) + } + PostById::::mutate(post_id, |stored_post_opt| { if let Some(stored_post) = stored_post_opt { stored_post.owner = new_owner.clone(); diff --git a/pallets/posts/tests/src/mock.rs b/pallets/posts/tests/src/mock.rs index d3dbce78..dfb715c3 100644 --- a/pallets/posts/tests/src/mock.rs +++ b/pallets/posts/tests/src/mock.rs @@ -154,7 +154,7 @@ parameter_types! { } pub struct MockEmptyDomainsProvider; impl DomainsProvider for MockEmptyDomainsProvider { - type DomainLength = MaxDomainLength; + type MaxDomainLength = MaxDomainLength; fn get_domain_owner(_domain: &[u8]) -> Result { Ok(ACCOUNT1) @@ -164,7 +164,7 @@ impl DomainsProvider for MockEmptyDomainsProvider { Ok(()) } - fn update_domain_owner(_domain: &[u8], _new_owner: &AccountId) -> DispatchResult { + fn do_update_domain_owner(_domain: &[u8], _new_owner: &AccountId) -> DispatchResult { Ok(()) } diff --git a/pallets/profiles/src/mock.rs b/pallets/profiles/src/mock.rs index 3c0ffc27..9ff9b664 100644 --- a/pallets/profiles/src/mock.rs +++ b/pallets/profiles/src/mock.rs @@ -89,7 +89,7 @@ mock! { impl SpacesProvider for Spaces { fn get_space_owner(_space_id: SpaceId) -> Result; - fn update_space_owner(_space_id: SpaceId, _new_owner: AccountId) -> DispatchResult; + fn do_update_space_owner(_space_id: SpaceId, _new_owner: AccountId) -> DispatchResult; fn create_space(_owner: &AccountId, _content: Content) -> Result; } diff --git a/pallets/space-ownership/src/lib.rs b/pallets/space-ownership/src/lib.rs index 33616989..b6769478 100644 --- a/pallets/space-ownership/src/lib.rs +++ b/pallets/space-ownership/src/lib.rs @@ -31,7 +31,7 @@ pub mod pallet { use subsocial_support::{PostId, SpaceId, SpacePermissionsInfo, traits::{CreatorStakingProvider, DomainsProvider, ProfileManager, SpacesProvider, PostsProvider, SpacePermissionsProvider}}; pub(crate) type DomainLengthOf = - <::DomainsProvider as DomainsProvider<::AccountId>>::DomainLength; + <::DomainsProvider as DomainsProvider<::AccountId>>::MaxDomainLength; #[derive(Encode, Decode, Clone, Eq, PartialEq, RuntimeDebugNoBound, TypeInfo, MaxEncodedLen)] #[scale_info(skip_type_params(T))] @@ -66,7 +66,7 @@ pub mod pallet { } /// The current storage version - const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + const STORAGE_VERSION: StorageVersion = StorageVersion::new(0); #[pallet::pallet] #[pallet::storage_version(STORAGE_VERSION)] @@ -170,14 +170,14 @@ pub mod pallet { let previous_owner = T::SpacesProvider::get_space_owner(space_id)?; Self::ensure_not_active_creator(space_id)?; - - T::SpacesProvider::update_space_owner(space_id, pending_owner.clone())?; + + T::SpacesProvider::do_update_space_owner(space_id, pending_owner.clone())?; T::ProfileManager::unlink_space_from_profile(&previous_owner, space_id); } OwnableEntity::Post(post_id) => - T::PostsProvider::update_post_owner(post_id, &pending_owner)?, + T::PostsProvider::do_update_post_owner(post_id, &pending_owner)?, OwnableEntity::Domain(domain) => - T::DomainsProvider::update_domain_owner(&domain, &pending_owner)?, + T::DomainsProvider::do_update_domain_owner(&domain, &pending_owner)?, } PendingOwnershipTransfers::::remove(&entity); diff --git a/pallets/spaces/src/lib.rs b/pallets/spaces/src/lib.rs index 6743155a..793f5e2d 100644 --- a/pallets/spaces/src/lib.rs +++ b/pallets/spaces/src/lib.rs @@ -432,9 +432,13 @@ pub mod pallet { Ok(space.owner) } - fn update_space_owner(space_id: SpaceId, new_owner: T::AccountId) -> DispatchResult { - Self::ensure_space_limit_not_reached(&new_owner)?; + fn do_update_space_owner(space_id: SpaceId, new_owner: T::AccountId) -> DispatchResult { let space = Pallet::::require_space(space_id)?; + if space.is_owner(&new_owner) { + return Ok(()); + } + + Self::ensure_space_limit_not_reached(&new_owner)?; ensure!( T::IsAccountBlocked::is_allowed_account(new_owner.clone(), space_id), diff --git a/pallets/support/src/traits/common.rs b/pallets/support/src/traits/common.rs index e6dfcd92..60ad69b6 100644 --- a/pallets/support/src/traits/common.rs +++ b/pallets/support/src/traits/common.rs @@ -36,7 +36,7 @@ pub trait SpacesProvider { fn get_space_owner(space_id: SpaceId) -> Result; - fn update_space_owner(space_id: SpaceId, new_owner: AccountId) -> DispatchResult; + fn do_update_space_owner(space_id: SpaceId, new_owner: AccountId) -> DispatchResult; fn create_space(owner: &AccountId, content: Content) -> Result; } @@ -56,13 +56,13 @@ impl CreatorStakingProvider for () { } pub trait DomainsProvider { - type DomainLength: frame_support::traits::Get; + type MaxDomainLength: frame_support::traits::Get; fn get_domain_owner(domain: &[u8]) -> Result; fn ensure_domain_owner(domain: &[u8], account: &AccountId) -> DispatchResult; - fn update_domain_owner(domain: &[u8], new_owner: &AccountId) -> DispatchResult; + fn do_update_domain_owner(domain: &[u8], new_owner: &AccountId) -> DispatchResult; #[cfg(feature = "runtime-benchmarks")] fn register_domain(owner: &AccountId, domain: &[u8]) -> Result, DispatchError>; @@ -73,7 +73,7 @@ pub trait PostsProvider { fn ensure_post_owner(post_id: PostId, account: &AccountId) -> DispatchResult; - fn update_post_owner(post_id: PostId, new_owner: &AccountId) -> DispatchResult; + fn do_update_post_owner(post_id: PostId, new_owner: &AccountId) -> DispatchResult; #[cfg(feature = "runtime-benchmarks")] fn create_post(owner: &AccountId, space_id: SpaceId, content: Content) -> Result; From 54601736709c68359d30c9beb38649cec5fb7587 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Wed, 27 Mar 2024 16:31:34 +0200 Subject: [PATCH 24/25] Update migration for ownership pallet --- pallets/space-ownership/src/migration.rs | 86 +++++++++--------------- 1 file changed, 33 insertions(+), 53 deletions(-) diff --git a/pallets/space-ownership/src/migration.rs b/pallets/space-ownership/src/migration.rs index 93a4f544..976720a5 100644 --- a/pallets/space-ownership/src/migration.rs +++ b/pallets/space-ownership/src/migration.rs @@ -16,13 +16,10 @@ const LOG_TARGET: &'static str = "runtime::ownership"; pub mod v1 { use frame_support::{ - migration::move_pallet, pallet_prelude::*, storage_alias, weights::Weight, + pallet_prelude::*, storage_alias, weights::Weight, }; - #[cfg(feature = "try-runtime")] - use frame_support::migration::storage_key_iter; - #[cfg(feature = "try-runtime")] use sp_io::hashing::twox_128; - use sp_runtime::Saturating; + use sp_io::KillStorageResult; use subsocial_support::SpaceId; @@ -39,48 +36,49 @@ pub mod v1 { { fn on_runtime_upgrade() -> Weight { let current_version = Pallet::::current_storage_version(); - let onchain_version = Pallet::::on_chain_storage_version(); let old_pallet_name = N::get(); + let old_pallet_prefix = twox_128(old_pallet_name.as_bytes()); + let old_pallet_has_data = sp_io::storage::next_key(&old_pallet_prefix).is_some(); + let new_pallet_name =

::name(); log::info!( target: LOG_TARGET, - "Running migration with current storage version {:?} / onchain {:?}", - current_version, - onchain_version + "Running migration to clean-up the old pallet name {}", + old_pallet_name, ); - if onchain_version == 0 && current_version == 1 { - current_version.put::>(); - + if old_pallet_has_data { if new_pallet_name == old_pallet_name { log::warn!( target: LOG_TARGET, - "new ownership name is equal to the old one, only bumping the version" + "new ownership name is equal to the old one, migration won't run" ); - return T::DbWeight::get().reads_writes(1, 1) + return T::DbWeight::get().reads(1) } - move_pallet(old_pallet_name.as_bytes(), new_pallet_name.as_bytes()); - - let mut migrated = 0u64; + current_version.put::>(); - for (space_id, account) in PendingSpaceOwner::::drain() { - PendingOwnershipTransfers::::insert( - OwnableEntity::Space(space_id), - account, - ); - migrated.saturating_inc(); + match sp_io::storage::clear_prefix(&old_pallet_prefix, None) { + KillStorageResult::SomeRemaining(remaining) => { + log::warn!( + target: LOG_TARGET, + "Some records from the old pallet {} have not been removed: {:?}", + old_pallet_name, + remaining + ); + } + KillStorageResult::AllRemoved(removed) => { + log::info!( + target: LOG_TARGET, + "Removed {} records from the old pallet {}", + removed, + old_pallet_name + ); + }, } - log::info!( - target: LOG_TARGET, - "Upgraded {} records, storage to version {:?}", - migrated, - current_version - ); - ::BlockWeights::get().max_block } else { log::info!( @@ -93,41 +91,22 @@ pub mod v1 { #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result, &'static str> { - let current_version = Pallet::::current_storage_version(); - let onchain_version = Pallet::::on_chain_storage_version(); let old_pallet_name = N::get().as_bytes(); let old_pallet_prefix = twox_128(old_pallet_name); - ensure!(onchain_version == 0 && current_version == 1, "migration from version 0 to 1."); ensure!( sp_io::storage::next_key(&old_pallet_prefix).is_some(), - "no data for the old pallet name has been detected" + "no data for the old pallet name has been detected; consider removing the migration" ); - // let prev_count = PendingSpaceOwner::::iter().count(); - let prev_count = storage_key_iter::(old_pallet_name, b"PendingSpaceOwner").count(); - Ok((prev_count as u32).encode()) + Ok(Vec::new()) } #[cfg(feature = "try-runtime")] - fn post_upgrade(prev_count: Vec) -> Result<(), &'static str> { - let prev_count: u32 = Decode::decode(&mut prev_count.as_slice()).expect( - "the state parameter should be something that was generated by pre_upgrade", - ); - let post_count = PendingOwnershipTransfers::::iter().count() as u32; - let old_storage_count = PendingSpaceOwner::::iter().count(); - + fn post_upgrade(_: Vec) -> Result<(), &'static str> { let old_pallet_name = N::get(); let new_pallet_name =

::name(); - ensure!( - prev_count == post_count, - "the records count before and after the migration should be the same" - ); - ensure!(old_storage_count.is_zero(), "all records should be migrated"); - - ensure!(Pallet::::on_chain_storage_version() == 1, "wrong storage version"); - // skip storage prefix checks for the same pallet names if new_pallet_name == old_pallet_name { return Ok(()); @@ -145,6 +124,7 @@ pub mod v1 { "old pallet data hasn't been removed" ); + // Assert nothing redundant is left in the new prefix. // NOTE: storage_version_key is already in the new prefix. let new_pallet_prefix = twox_128(new_pallet_name.as_bytes()); let new_pallet_prefix_iter = frame_support::storage::KeyPrefixIterator::new( @@ -152,7 +132,7 @@ pub mod v1 { new_pallet_prefix.to_vec(), |_| Ok(()), ); - assert_eq!(new_pallet_prefix_iter.count(), (prev_count + 1) as usize); + assert_eq!(new_pallet_prefix_iter.count(), 1); Ok(()) } From a740d7e5f8b7a6bc2368f3e4ff3cfec57e4dfe51 Mon Sep 17 00:00:00 2001 From: Vlad Proshchavaiev <32250097+F3Joule@users.noreply.github.com> Date: Wed, 27 Mar 2024 16:34:56 +0200 Subject: [PATCH 25/25] Fix migration after review --- pallets/space-ownership/src/migration.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/pallets/space-ownership/src/migration.rs b/pallets/space-ownership/src/migration.rs index 976720a5..a96551c3 100644 --- a/pallets/space-ownership/src/migration.rs +++ b/pallets/space-ownership/src/migration.rs @@ -16,19 +16,13 @@ const LOG_TARGET: &'static str = "runtime::ownership"; pub mod v1 { use frame_support::{ - pallet_prelude::*, storage_alias, weights::Weight, + pallet_prelude::*, weights::Weight, }; use sp_io::hashing::twox_128; use sp_io::KillStorageResult; - use subsocial_support::SpaceId; - use super::*; - #[storage_alias] - pub type PendingSpaceOwner = - StorageMap, Twox64Concat, SpaceId, ::AccountId>; - pub struct MigrateToV1(sp_std::marker::PhantomData<(T, P, N)>); impl> OnRuntimeUpgrade @@ -72,7 +66,7 @@ pub mod v1 { KillStorageResult::AllRemoved(removed) => { log::info!( target: LOG_TARGET, - "Removed {} records from the old pallet {}", + "Removed all the {} records from the old pallet {}", removed, old_pallet_name );