fix: pool fee event dispatch order (#1809)

* fix: pool fee event dispatch order

* refactor: tranche asset registration order

* refactor: pool registration metadata event dispatch (#1811)

* fix: pool creation

* fix: build docs

pallets/pool-registry/src/lib.rs

@@ -272,18 +272,8 @@ pub mod pallet {
 				Pools::<T>::insert(pool_id, PoolRegistrationStatus::Registered);
 			}
 
-			if let Some(m) = metadata.clone() {
-				let checked_metadata: BoundedVec<u8, T::MaxSizeMetadata> =
-					m.try_into().map_err(|_| Error::<T>::BadMetadata)?;
-
-				PoolMetadata::<T>::insert(
-					pool_id,
-					PoolMetadataOf::<T> {
-						metadata: checked_metadata,
-					},
-				);
-			}
-
+			// For SubQuery, pool registration event should be dispatched before MetadataSet
+			// one
 			T::ModifyPool::create(
 				admin,
 				depositor,
@@ -295,6 +285,8 @@ pub mod pallet {
 			)
 			.map(|_| Self::deposit_event(Event::Registered { pool_id }))?;
 
+			Self::do_set_metadata(pool_id, metadata.unwrap_or_default())?;
+
 			T::ModifyWriteOffPolicy::update(pool_id, write_off_policy)
 		}
 
@@ -412,22 +404,14 @@ pub mod pallet {
 				BadOrigin,
 			);
 
-			let checked_metadata = Self::do_set_metadata(pool_id, metadata)?;
-
-			Self::deposit_event(Event::MetadataSet {
-				pool_id,
-				metadata: checked_metadata,
-			});
+			Self::do_set_metadata(pool_id, metadata)?;
 
 			Ok(())
 		}
 	}
 
 	impl<T: Config> Pallet<T> {
-		pub(crate) fn do_set_metadata(
-			pool_id: T::PoolId,
-			metadata: Vec<u8>,
-		) -> Result<BoundedVec<u8, T::MaxSizeMetadata>, DispatchError> {
+		pub(crate) fn do_set_metadata(pool_id: T::PoolId, metadata: Vec<u8>) -> DispatchResult {
 			let checked_metadata: BoundedVec<u8, T::MaxSizeMetadata> =
 				metadata.try_into().map_err(|_| Error::<T>::BadMetadata)?;
 
@@ -438,7 +422,12 @@ pub mod pallet {
 				},
 			);
 
-			Ok(checked_metadata)
+			Self::deposit_event(Event::MetadataSet {
+				pool_id,
+				metadata: checked_metadata,
+			});
+
+			Ok(())
 		}
 	}
 
@@ -454,7 +443,7 @@ pub mod pallet {
 		}
 
 		fn set_pool_metadata(pool_id: Self::PoolId, metadata: Vec<u8>) -> DispatchResult {
-			Self::do_set_metadata(pool_id, metadata).map(|_| ())
+			Self::do_set_metadata(pool_id, metadata)
 		}
 
 		fn get_tranche_token_metadata(

pallets/pool-registry/src/tests.rs

@@ -20,7 +20,17 @@ use pallet_pool_system::{
 };
 use staging_xcm::VersionedMultiLocation;
 
-use crate::{mock::*, pallet, pallet::Error, PoolMetadataOf};
+use crate::{mock::*, pallet, pallet::Error, Event, PoolMetadataOf};
+
+fn find_metadata_event(pool_id: u64, metadata: BoundedVec<u8, MaxSizeMetadata>) -> Option<usize> {
+	System::events().iter().position(|e| match &e.event {
+		RuntimeEvent::PoolRegistry(Event::MetadataSet {
+			pool_id: id,
+			metadata: m,
+		}) if pool_id == *id && metadata == *m => true,
+		_ => false,
+	})
+}
 
 #[test]
 fn update_pool() {
@@ -67,7 +77,7 @@ fn register_pool_and_set_metadata() {
 			let hash = "QmUTwA6RTUb1FbJCeM1D4G4JaWHAbPehK8WwCfykJixjm3" // random IPFS hash, for test purposes
 				.as_bytes()
 				.to_vec();
-			let metadata = Some(hash);
+			let metadata = Some(hash.clone());
 
 			MockWriteOffPolicy::mock_update(|_, _| Ok(()));
 
@@ -86,6 +96,21 @@ fn register_pool_and_set_metadata() {
 			let registered_metadata = PoolRegistry::get_pool_metadata(pool_id);
 
 			assert_eq!(registered_metadata.unwrap().metadata, metadata.unwrap());
+
+			let pos_reg = System::events()
+				.iter()
+				.position(|e| match e.event {
+					RuntimeEvent::PoolRegistry(Event::Registered { pool_id: id })
+						if pool_id == id =>
+					{
+						true
+					}
+					_ => false,
+				})
+				.expect("Pool registered; qed");
+			let pos_metadata = find_metadata_event(pool_id, BoundedVec::truncate_from(hash))
+				.expect("Metadata not empty; qed");
+			assert!(pos_reg < pos_metadata);
 		})
 }
 
@@ -96,14 +121,17 @@ fn set_metadata() {
 		.execute_with(|| {
 			let pool_owner = 0u64;
 			let pool_id = 0;
+			let metadata = "QmUTwA6RTUb1FbJCeM1D4G4JaMHAbPehK6WwCfykJixjm3" // random IPFS hash, for test purposes
+				.as_bytes()
+				.to_vec();
 
 			assert_ok!(PoolRegistry::set_metadata(
 				RuntimeOrigin::signed(pool_owner),
 				pool_id,
-				"QmUTwA6RTUb1FbJCeM1D4G4JaMHAbPehK6WwCfykJixjm3" // random IPFS hash, for test purposes
-					.as_bytes()
-					.to_vec()
+				metadata.clone(),
 			));
+
+			assert!(find_metadata_event(pool_id, BoundedVec::truncate_from(metadata)).is_some())
 		})
 }
 
@@ -113,16 +141,74 @@ fn trait_pool_metadata_set_pool_metadata() {
 		.build()
 		.execute_with(|| {
 			let pool_id = 0;
+			let metadata = "QmUTwA6RTUb1FbJCeM1D4G4JaMHAbPehK6WwCfykJixjm3" // random IPFS hash, for test purposes
+				.as_bytes()
+				.to_vec();
 
 			assert_ok!(<PoolRegistry as PoolMetadata<
 				Balance,
 				VersionedMultiLocation,
-			>>::set_pool_metadata(
+			>>::set_pool_metadata(pool_id, metadata.clone()));
+
+			assert!(find_metadata_event(pool_id, BoundedVec::truncate_from(metadata)).is_some())
+		})
+}
+
+#[test]
+fn set_excess_metadata_fails() {
+	TestExternalitiesBuilder::default()
+		.build()
+		.execute_with(|| {
+			let pool_id = 0;
+
+			assert_noop!(
+				PoolRegistry::do_set_metadata(
+					pool_id,
+					(0..=MaxSizeMetadata::get())
+						.into_iter()
+						.map(|x| x as u8)
+						.collect::<Vec<u8>>()
+				),
+				Error::<Test>::BadMetadata
+			);
+		})
+}
+
+#[test]
+fn register_pool_empty_metadata() {
+	TestExternalitiesBuilder::default()
+		.build()
+		.execute_with(|| {
+			let pool_owner = 0u64;
+			let pool_id = 0;
+
+			let tranches_inputs = vec![TrancheInput {
+				tranche_type: TrancheType::Residual,
+				seniority: None,
+				metadata: TrancheMetadata {
+					token_name: BoundedVec::default(),
+					token_symbol: BoundedVec::default(),
+				},
+			}];
+
+			let currency = AUSD_CURRENCY_ID;
+			let max_reserve: u128 = 10_000 * 1_000_000_000;
+
+			MockWriteOffPolicy::mock_update(|_, _| Ok(()));
+
+			assert_ok!(PoolRegistry::register(
+				RuntimeOrigin::signed(pool_owner),
+				pool_owner,
 				pool_id,
-				"QmUTwA6RTUb1FbJCeM1D4G4JaMHAbPehK6WwCfykJixjm3" // random IPFS hash, for test purposes
-					.as_bytes()
-					.to_vec()
+				tranches_inputs,
+				currency,
+				max_reserve,
+				None,
+				(),
+				vec![]
 			));
+
+			assert!(find_metadata_event(pool_id, BoundedVec::default()).is_some())
 		})
 }
 

pallets/pool-system/src/impls.rs

@@ -154,31 +154,9 @@ impl<T: Config> PoolMutate<T::AccountId, T::PoolId> for Pallet<T> {
 			T::MaxTranches,
 		>::from_input::<T::StringLimit>(pool_id, tranche_inputs.clone(), now)?;
 
-		for (tranche, tranche_input) in tranches.tranches.iter().zip(&tranche_inputs) {
-			let token_name = tranche_input.metadata.token_name.clone();
-			let token_symbol = tranche_input.metadata.token_symbol.clone();
-
-			// The decimals of the tranche token need to match the decimals of the pool
-			// currency. Otherwise, we'd always need to convert investments to the decimals
-			// of tranche tokens and vice versa
-			let decimals = match T::AssetRegistry::metadata(&currency) {
-				Some(metadata) => metadata.decimals,
-				None => return Err(Error::<T>::MetadataForCurrencyNotFound.into()),
-			};
-
-			let metadata = tranche.create_asset_metadata(decimals, token_name, token_symbol);
-
-			T::AssetRegistry::register_asset(Some(tranche.currency.into()), metadata)
-				.map_err(|_| Error::<T>::FailedToRegisterTrancheMetadata)?;
-		}
-
-		for (fee_bucket, pool_fee) in pool_fees.into_iter() {
-			T::PoolFees::add_fee(pool_id, fee_bucket, pool_fee)?;
-		}
-
 		let pool_details = PoolDetails {
 			currency,
-			tranches,
+			tranches: tranches.clone(),
 			status: PoolStatus::Open,
 			epoch: EpochState {
 				current: One::one(),
@@ -198,13 +176,43 @@ impl<T: Config> PoolMutate<T::AccountId, T::PoolId> for Pallet<T> {
 
 		Pool::<T>::insert(pool_id, pool_details.clone());
 
+		// For SubQuery, pool creation event should be dispatched before related events
+		let ids: Vec<T::TrancheCurrency> = tranches
+			.tranches
+			.clone()
+			.into_iter()
+			.map(|tranche| tranche.currency)
+			.collect();
 		Self::deposit_event(Event::Created {
 			admin: admin.clone(),
 			depositor,
 			pool_id,
-			essence: pool_details.essence::<T::AssetRegistry, T::Balance, T::StringLimit>()?,
+			essence: pool_details
+				.essence_from_tranche_input::<T::StringLimit>(ids, tranche_inputs.clone())?,
 		});
 
+		for (tranche, tranche_input) in tranches.tranches.iter().zip(&tranche_inputs) {
+			let token_name = tranche_input.metadata.token_name.clone();
+			let token_symbol = tranche_input.metadata.token_symbol.clone();
+
+			// The decimals of the tranche token need to match the decimals of the pool
+			// currency. Otherwise, we'd always need to convert investments to the decimals
+			// of tranche tokens and vice versa
+			let decimals = match T::AssetRegistry::metadata(&currency) {
+				Some(metadata) => metadata.decimals,
+				None => return Err(Error::<T>::MetadataForCurrencyNotFound.into()),
+			};
+
+			let metadata = tranche.create_asset_metadata(decimals, token_name, token_symbol);
+
+			T::AssetRegistry::register_asset(Some(tranche.currency.into()), metadata)
+				.map_err(|_| Error::<T>::FailedToRegisterTrancheMetadata)?;
+		}
+
+		for (fee_bucket, pool_fee) in pool_fees.into_iter() {
+			T::PoolFees::add_fee(pool_id, fee_bucket, pool_fee)?;
+		}
+
 		T::Permission::add(
 			PermissionScope::Pool(pool_id),
 			admin,

pallets/pool-system/src/lib.rs

@@ -1087,7 +1087,8 @@ pub mod pallet {
 				let pool = pool.as_mut().ok_or(Error::<T>::NoSuchPool)?;
 
 				// Prepare PoolEssence struct for sending out UpdateExecuted event
-				let old_pool = pool.essence::<T::AssetRegistry, T::Balance, T::StringLimit>()?;
+				let old_pool =
+					pool.essence_from_registry::<T::AssetRegistry, T::Balance, T::StringLimit>()?;
 
 				if let Change::NewValue(min_epoch_time) = changes.min_epoch_time {
 					pool.parameters.min_epoch_time = min_epoch_time;
@@ -1142,7 +1143,8 @@ pub mod pallet {
 				Self::deposit_event(Event::Updated {
 					id: *pool_id,
 					old: old_pool,
-					new: pool.essence::<T::AssetRegistry, T::Balance, T::StringLimit>()?,
+					new: pool
+						.essence_from_registry::<T::AssetRegistry, T::Balance, T::StringLimit>()?,
 				});
 
 				ScheduledUpdate::<T>::remove(pool_id);