fix: change serverId dataType (#145)

* fix: added new storage in server pallet * fix: benchmarking server pallet * fix: added new tests server pallet * fix: migration storage server pallet * fix: change serverId data type in tipping pallet * fix: benchmarking tipping pallet * fix: removed register from tipping pallet test * fix: migration storage tipping pallet * chore: bump version 2.1.9
myriadsocial · Sep 15, 2022 · 6283002 · 6283002
1 parent 69f127d
commit 6283002
Show file tree

Hide file tree

Showing 25 changed files with 388 additions and 600 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/node/Cargo.toml b/node/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = 'myriad'
-version = '2.1.8'
+version = '2.1.9'
 edition = '2021'
 license = 'AGPL-3.0'
 authors = ['Myriad Dev Team <[email protected]>']

diff --git a/pallets/server/Cargo.toml b/pallets/server/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = 'pallet-server'
-version = '2.1.8'
+version = '2.1.9'
 edition = '2021'
 license = 'AGPL-3.0'
 authors = ['Myriad Dev Team <[email protected]>']

diff --git a/pallets/server/src/benchmarking.rs b/pallets/server/src/benchmarking.rs
@@ -5,7 +5,6 @@ use super::*;
 #[allow(unused)]
 use crate::{Pallet as Server, ServerInterface};
 use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller};
-use frame_support::sp_runtime::SaturatedConversion;
 use frame_system::RawOrigin;
 use sp_std::vec;
 
@@ -21,10 +20,8 @@ benchmarks! {
 		let caller: T::AccountId = whitelisted_caller();
 		let caller_origin = <T as frame_system::Config>::Origin::from(RawOrigin::Signed(caller.clone()));
 
-		let server_id = 0;
-		let server_name = "myriad".as_bytes().to_vec();
+		let server_id = 0_u64;
 		let server_api_url = "https://api.dev.myriad.social".as_bytes().to_vec();
-		let server_web_url = "https://app.dev.myriad.social".as_bytes().to_vec();
 
 		let _ = Server::<T>::register(caller_origin.clone(), server_api_url);
 		let new_api_url = "https://api.testnet.myriad.social".as_bytes().to_vec();
@@ -34,10 +31,8 @@ benchmarks! {
 		let caller: T::AccountId = whitelisted_caller();
 		let caller_origin = <T as frame_system::Config>::Origin::from(RawOrigin::Signed(caller.clone()));
 
-		let server_id = 0;
-		let server_name = "myriad".as_bytes().to_vec();
+		let server_id = 0_u64;
 		let server_api_url = "https://api.dev.myriad.social".as_bytes().to_vec();
-		let server_web_url = "https://app.dev.myriad.social".as_bytes().to_vec();
 
 		let _ = Server::<T>::register(caller_origin.clone(), server_api_url);
 		let new_owner: T::AccountId = account("new_owner", 0, SEED);
@@ -47,10 +42,8 @@ benchmarks! {
 		let caller: T::AccountId = whitelisted_caller();
 		let caller_origin = <T as frame_system::Config>::Origin::from(RawOrigin::Signed(caller.clone()));
 
-		let server_id = 0;
-		let server_name = "myriad".as_bytes().to_vec();
+		let server_id = 0_u64;
 		let server_api_url = "https://api.dev.myriad.social".as_bytes().to_vec();
-		let server_web_url = "https://app.dev.myriad.social".as_bytes().to_vec();
 
 		let _ = Server::<T>::register(caller_origin.clone(), server_api_url);
 	}: _(RawOrigin::Signed(caller), server_id)

diff --git a/pallets/server/src/functions.rs b/pallets/server/src/functions.rs
@@ -1,6 +1,14 @@
 use crate::*;
 
 impl<T: Config> Pallet<T> {
+	pub fn do_api_url_exist(api_url: &[u8]) -> Result<(), Error<T>> {
+		if Self::server_by_api_url(api_url).is_some() {
+			return Err(Error::<T>::AlreadyExists)
+		}
+
+		Ok(())
+	}
+
 	pub fn do_mutate_server(
 		server_id: u64,
 		owner: &T::AccountId,
@@ -14,7 +22,10 @@ impl<T: Config> Pallet<T> {
 
 				let updated_server = match data {
 					ServerDataKind::Owner(new_owner) => server.clone().set_owner(new_owner),
-					ServerDataKind::ApiUrl(new_url) => server.clone().set_api_url(new_url),
+					ServerDataKind::ApiUrl(new_url) => {
+						ServerByApiUrl::<T>::swap(server.get_api_url(), &new_url);
+						server.clone().set_api_url(new_url)
+					},
 				};
 
 				ServerByOwner::<T>::insert(owner, server_id, &updated_server);

diff --git a/pallets/server/src/impl_server.rs b/pallets/server/src/impl_server.rs
@@ -9,6 +9,8 @@ impl<T: Config> ServerInterface<T> for Pallet<T> {
 	}
 
 	fn register(owner: &T::AccountId, api_url: &[u8]) -> Result<Self::Server, Self::Error> {
+		Self::do_api_url_exist(api_url)?;
+
 		let count = Self::server_count();
 		let index = Self::server_index();
 
@@ -20,6 +22,7 @@ impl<T: Config> ServerInterface<T> for Pallet<T> {
 		ServerCount::<T>::set(updated_count);
 		ServerIndex::<T>::set(updated_index);
 		ServerById::<T>::insert(index, &server);
+		ServerByApiUrl::<T>::insert(api_url, index);
 		ServerByOwner::<T>::insert(owner, index, &server);
 
 		Ok(server)
@@ -42,6 +45,7 @@ impl<T: Config> ServerInterface<T> for Pallet<T> {
 		owner: &T::AccountId,
 		new_api_url: &[u8],
 	) -> Result<(), Self::Error> {
+		Self::do_api_url_exist(new_api_url)?;
 		Self::do_mutate_server(server_id, owner, &ServerDataKind::ApiUrl(new_api_url.to_vec()))?;
 
 		Ok(())
@@ -59,9 +63,10 @@ impl<T: Config> ServerInterface<T> for Pallet<T> {
 
 		let count = Self::server_count().checked_sub(1).ok_or(Error::<T>::Overflow)?;
 
+		ServerCount::<T>::set(count);
 		ServerById::<T>::remove(server_id);
 		ServerByOwner::<T>::remove(owner, server_id);
-		ServerCount::<T>::set(count);
+		ServerByApiUrl::<T>::remove(server.get_api_url());
 
 		Ok(())
 	}

diff --git a/pallets/server/src/lib.rs b/pallets/server/src/lib.rs
@@ -23,7 +23,7 @@ pub use weights::WeightInfo;
 use frame_support::traits::StorageVersion;
 
 /// The current storage version.
-const STORAGE_VERSION: StorageVersion = StorageVersion::new(3);
+const STORAGE_VERSION: StorageVersion = StorageVersion::new(4);
 
 #[frame_support::pallet]
 pub mod pallet {
@@ -47,7 +47,7 @@ pub mod pallet {
 
 	#[pallet::storage]
 	#[pallet::getter(fn server_count)]
-	pub type ServerCount<T> = StorageValue<_, ServerId, ValueQuery>;
+	pub type ServerCount<T> = StorageValue<_, u64, ValueQuery>;
 
 	#[pallet::storage]
 	#[pallet::getter(fn server_index)]
@@ -57,6 +57,10 @@ pub mod pallet {
 	#[pallet::getter(fn server_by_id)]
 	pub(super) type ServerById<T: Config> = StorageMap<_, Blake2_128Concat, ServerId, ServerOf<T>>;
 
+	#[pallet::storage]
+	#[pallet::getter(fn server_by_api_url)]
+	pub(super) type ServerByApiUrl<T: Config> = StorageMap<_, Blake2_128Concat, ApiUrl, ServerId>;
+
 	#[pallet::storage]
 	#[pallet::getter(fn server_by_owner)]
 	pub(super) type ServerByOwner<T: Config> = StorageDoubleMap<

diff --git a/pallets/server/src/migrations.rs b/pallets/server/src/migrations.rs
@@ -1,5 +1,5 @@
 use crate::{
-	AccountIdOf, Config, Pallet, Server as NewServer, ServerById as NewServerById,
+	AccountIdOf, Config, Pallet, Server as NewServer, ServerByApiUrl, ServerById as NewServerById,
 	ServerByOwner as NewServerByOwner, ServerCount as NewServerCount, ServerId,
 	ServerIndex as NewServerIndex, ServerOf,
 };
@@ -29,6 +29,11 @@ pub fn migrate<T: Config>() -> Weight {
 		version = StorageVersion::new(3);
 	}
 
+	if version == 3 {
+		weight = weight.saturating_add(versions::v4::migrate::<T>());
+		version = StorageVersion::new(4);
+	}
+
 	version.put::<Pallet<T>>();
 	weight
 }
@@ -149,4 +154,22 @@ mod versions {
 			weight
 		}
 	}
+
+	pub mod v4 {
+		use super::*;
+
+		pub fn migrate<T: Config>() -> Weight {
+			let mut weight = T::DbWeight::get().writes(1);
+
+			NewServerById::<T>::translate(|server_id: ServerId, server: ServerOf<T>| {
+				weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 1));
+
+				ServerByApiUrl::<T>::insert(server.get_api_url(), server_id);
+
+				Some(server)
+			});
+
+			weight
+		}
+	}
 }
diff --git a/pallets/server/src/tests.rs b/pallets/server/src/tests.rs
@@ -12,10 +12,11 @@ fn register_works() {
 
 		let server = pallet_server::Server::new(server_id, &owner, &api_url);
 
-		assert_ok!(Server::register(owner_origin, api_url));
+		assert_ok!(Server::register(owner_origin, api_url.clone()));
 
 		assert_eq!(Server::server_by_id(server_id), Some(server.clone()));
 		assert_eq!(Server::server_by_owner(owner, server_id), Some(server));
+		assert_eq!(Server::server_by_api_url(api_url), Some(server_id));
 		assert_eq!(Server::server_count(), 1);
 		assert_eq!(Server::server_index(), 1);
 	})
@@ -38,7 +39,7 @@ pub fn transfer_owner_works() {
 
 		assert_eq!(Server::server_by_id(server_id), Some(server.clone()));
 		assert_eq!(Server::server_by_owner(owner, server_id), None);
-		assert_eq!(Server::server_by_owner(new_owner, server_id), Some(server.clone()));
+		assert_eq!(Server::server_by_owner(new_owner, server_id), Some(server));
 	})
 }
 
@@ -54,10 +55,12 @@ pub fn change_api_url_works() {
 		let new_api_url = "https://api.testnet.myriad.social".as_bytes().to_vec();
 		let server = pallet_server::Server::new(server_id, &owner, &new_api_url);
 
-		assert_ok!(Server::register(Origin::signed(owner), api_url));
-		assert_ok!(Server::update_api_url(owner_origin, 0, new_api_url,));
+		assert_ok!(Server::register(Origin::signed(owner), api_url.clone()));
+		assert_ok!(Server::update_api_url(owner_origin, 0, new_api_url.clone()));
 
+		assert_eq!(Server::server_by_api_url(api_url), None);
 		assert_eq!(Server::server_by_id(server_id), Some(server));
+		assert_eq!(Server::server_by_api_url(new_api_url), Some(server_id));
 	})
 }
 
@@ -70,12 +73,34 @@ pub fn deregister_works() {
 		let server_id = 0_u64;
 		let api_url = "https://api.dev.myriad.social".as_bytes().to_vec();
 
-		assert_ok!(Server::register(Origin::signed(owner), api_url));
+		assert_ok!(Server::register(Origin::signed(owner), api_url.clone()));
 		assert_ok!(Server::unregister(owner_origin, 0));
 
 		assert_eq!(Server::server_by_id(server_id), None);
 		assert_eq!(Server::server_by_owner(owner, server_id), None);
+		assert_eq!(Server::server_by_api_url(api_url), None);
 		assert_eq!(Server::server_count(), 0);
+		assert_eq!(Server::server_index(), 1);
+	})
+}
+
+#[test]
+pub fn cant_register_when_api_url_exist() {
+	ExternalityBuilder::build().execute_with(|| {
+		let owner = 1;
+		let owner_origin = Origin::signed(owner);
+		let api_url = b"https://api.dev.myriad.social".to_vec();
+
+		assert_ok!(Server::register(owner_origin, api_url));
+
+		let other_owner = 2;
+		let other_owner_origin = Origin::signed(other_owner);
+		let other_api_url = b"https://api.dev.myriad.social".to_vec();
+
+		assert_noop!(
+			Server::register(other_owner_origin, other_api_url),
+			Error::<Test>::AlreadyExists,
+		);
 	})
 }
 
@@ -146,7 +171,7 @@ pub fn cant_change_api_url_when_not_owner() {
 
 		let fake_owner = 3;
 		let fake_owner_origin = Origin::signed(fake_owner);
-		let new_api_url = "https://api.dev.myriad.social".as_bytes().to_vec();
+		let new_api_url = "https://api.testnet.myriad.social".as_bytes().to_vec();
 
 		assert_noop!(
 			Server::update_api_url(fake_owner_origin, server_id, new_api_url),
@@ -155,6 +180,32 @@ pub fn cant_change_api_url_when_not_owner() {
 	})
 }
 
+#[test]
+pub fn cant_change_api_url_when_api_url_exist() {
+	ExternalityBuilder::build().execute_with(|| {
+		let owner = 1;
+		let owner_origin = Origin::signed(owner);
+
+		let server_id = 0;
+		let api_url = b"https://api.dev.myriad.social".to_vec();
+
+		assert_ok!(Server::register(owner_origin.clone(), api_url));
+
+		let other_owner = 2;
+		let other_owner_origin = Origin::signed(other_owner);
+		let other_api_url = b"https://api.testnet.myriad.social".to_vec();
+
+		assert_ok!(Server::register(other_owner_origin, other_api_url));
+
+		let new_api_url = b"https://api.testnet.myriad.social".to_vec();
+
+		assert_noop!(
+			Server::update_api_url(owner_origin, server_id, new_api_url),
+			Error::<Test>::AlreadyExists,
+		);
+	})
+}
+
 #[test]
 pub fn cant_deregister_when_server_id_not_exist() {
 	ExternalityBuilder::build().execute_with(|| {

diff --git a/pallets/server/src/types.rs b/pallets/server/src/types.rs
@@ -73,3 +73,4 @@ pub enum OperatorKind {
 pub type AccountIdOf<T> = <T as frame_system::Config>::AccountId;
 pub type ServerOf<T> = Server<AccountIdOf<T>>;
 pub type ServerId = u64;
+pub type ApiUrl = Vec<u8>;