Apply final suggestions after code review

Co-Authored-By: Alex Siman <siman@users.noreply.github.com>

pallets/creator-staking/src/tests/mock.rs

@@ -161,7 +161,7 @@ mock! {
     impl SpacesProvider<AccountId, SpaceId> for Spaces {
         fn get_space_owner(_space_id: SpaceId) -> Result<AccountId, DispatchError>;
 
-        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<SpaceId, DispatchError>;
     }

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::<T>::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<DomainMeta<T>, DispatchError> {
-            ensure!(domain.len() <= T::MaxDomainLength::get() as usize, Error::<T>::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<T> 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<T: Config> DomainsProvider<T::AccountId> for Pallet<T> {
-        type DomainLength = T::MaxDomainLength;
+        type MaxDomainLength = T::MaxDomainLength;
 
         fn get_domain_owner(domain: &[u8]) -> Result<T::AccountId, DispatchError> {
             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<Vec<u8>, DispatchError> {
-            ensure!(domain.len() <= T::MaxDomainLength::get() as usize, Error::<T>::DomainIsTooLong);
+            Self::ensure_valid_domain_length(domain)?;
             let domain_lc = Self::lower_domain_then_bound(domain);
 
             Self::do_register_domain(

pallets/posts/src/functions.rs

@@ -472,7 +472,12 @@ impl<T: Config> PostsProvider<T::AccountId> for Pallet<T> {
         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::<T>::mutate(post_id, |stored_post_opt| {
             if let Some(stored_post) = stored_post_opt {
                 stored_post.owner = new_owner.clone();

pallets/posts/tests/src/mock.rs

@@ -154,7 +154,7 @@ parameter_types! {
 }
 pub struct MockEmptyDomainsProvider;
 impl DomainsProvider<AccountId> for MockEmptyDomainsProvider {
-    type DomainLength = MaxDomainLength;
+    type MaxDomainLength = MaxDomainLength;
 
     fn get_domain_owner(_domain: &[u8]) -> Result<AccountId, DispatchError> {
         Ok(ACCOUNT1)
@@ -164,7 +164,7 @@ impl DomainsProvider<AccountId> 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(())
     }
 

pallets/profiles/src/mock.rs

@@ -89,7 +89,7 @@ mock! {
     impl SpacesProvider<AccountId, SpaceId> for Spaces {
         fn get_space_owner(_space_id: SpaceId) -> Result<AccountId, DispatchError>;
 
-        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<SpaceId, DispatchError>;
     }

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<T> = 
-        <<T as Config>::DomainsProvider as DomainsProvider<<T as frame_system::Config>::AccountId>>::DomainLength;
+        <<T as Config>::DomainsProvider as DomainsProvider<<T as frame_system::Config>::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::<T>::remove(&entity);

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::<T>::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),

pallets/support/src/traits/common.rs

@@ -36,7 +36,7 @@ pub trait SpacesProvider<AccountId, SpaceId> {
 
     fn get_space_owner(space_id: SpaceId) -> Result<AccountId, DispatchError>;
 
-    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<SpaceId, DispatchError>;
 }
@@ -56,13 +56,13 @@ impl<AccountId> CreatorStakingProvider<AccountId> for () {
 }
 
 pub trait DomainsProvider<AccountId> {
-    type DomainLength: frame_support::traits::Get<u32>;
+    type MaxDomainLength: frame_support::traits::Get<u32>;
 
     fn get_domain_owner(domain: &[u8]) -> Result<AccountId, DispatchError>;
 
     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<sp_std::vec::Vec<u8>, DispatchError>;
@@ -73,7 +73,7 @@ pub trait PostsProvider<AccountId> {
 
     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<PostId, DispatchError>;