From fbec2ed0691c4d8ea6c1927c7136ba6024d7cde4 Mon Sep 17 00:00:00 2001 From: William Freudenberger Date: Tue, 23 Apr 2024 18:05:40 +0200 Subject: [PATCH] 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 | 37 ++++----- pallets/pool-registry/src/tests.rs | 104 +++++++++++++++++++++++--- pallets/pool-system/src/impls.rs | 56 ++++++++------ pallets/pool-system/src/lib.rs | 6 +- pallets/pool-system/src/pool_types.rs | 54 +++++++++++-- pallets/pool-system/src/tests/mod.rs | 22 +++++- 6 files changed, 211 insertions(+), 68 deletions(-) diff --git a/pallets/pool-registry/src/lib.rs b/pallets/pool-registry/src/lib.rs index 332296ec75..8f9dc5fd28 100644 --- a/pallets/pool-registry/src/lib.rs +++ b/pallets/pool-registry/src/lib.rs @@ -272,18 +272,8 @@ pub mod pallet { Pools::::insert(pool_id, PoolRegistrationStatus::Registered); } - if let Some(m) = metadata.clone() { - let checked_metadata: BoundedVec = - m.try_into().map_err(|_| Error::::BadMetadata)?; - - PoolMetadata::::insert( - pool_id, - PoolMetadataOf:: { - 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 Pallet { - pub(crate) fn do_set_metadata( - pool_id: T::PoolId, - metadata: Vec, - ) -> Result, DispatchError> { + pub(crate) fn do_set_metadata(pool_id: T::PoolId, metadata: Vec) -> DispatchResult { let checked_metadata: BoundedVec = metadata.try_into().map_err(|_| Error::::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) -> DispatchResult { - Self::do_set_metadata(pool_id, metadata).map(|_| ()) + Self::do_set_metadata(pool_id, metadata) } fn get_tranche_token_metadata( diff --git a/pallets/pool-registry/src/tests.rs b/pallets/pool-registry/src/tests.rs index 78468b5e68..52d2836166 100644 --- a/pallets/pool-registry/src/tests.rs +++ b/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) -> Option { + 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!(>::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::>() + ), + Error::::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()) }) } diff --git a/pallets/pool-system/src/impls.rs b/pallets/pool-system/src/impls.rs index a0cb34ae6a..da2a469df2 100644 --- a/pallets/pool-system/src/impls.rs +++ b/pallets/pool-system/src/impls.rs @@ -154,31 +154,9 @@ impl PoolMutate for Pallet { T::MaxTranches, >::from_input::(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(¤cy) { - Some(metadata) => metadata.decimals, - None => return Err(Error::::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::::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 PoolMutate for Pallet { Pool::::insert(pool_id, pool_details.clone()); + // For SubQuery, pool creation event should be dispatched before related events + let ids: Vec = 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::()?, + essence: pool_details + .essence_from_tranche_input::(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(¤cy) { + Some(metadata) => metadata.decimals, + None => return Err(Error::::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::::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, diff --git a/pallets/pool-system/src/lib.rs b/pallets/pool-system/src/lib.rs index 17fe6ff76b..7a949ed3bc 100644 --- a/pallets/pool-system/src/lib.rs +++ b/pallets/pool-system/src/lib.rs @@ -1087,7 +1087,8 @@ pub mod pallet { let pool = pool.as_mut().ok_or(Error::::NoSuchPool)?; // Prepare PoolEssence struct for sending out UpdateExecuted event - let old_pool = pool.essence::()?; + let old_pool = + pool.essence_from_registry::()?; 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::()?, + new: pool + .essence_from_registry::()?, }); ScheduledUpdate::::remove(pool_id); diff --git a/pallets/pool-system/src/pool_types.rs b/pallets/pool-system/src/pool_types.rs index 8eab01c58a..c648fedef1 100644 --- a/pallets/pool-system/src/pool_types.rs +++ b/pallets/pool-system/src/pool_types.rs @@ -29,7 +29,7 @@ use sp_runtime::{ use sp_std::{cmp::PartialEq, vec::Vec}; use crate::tranches::{ - EpochExecutionTranches, TrancheEssence, TrancheSolution, TrancheUpdate, Tranches, + EpochExecutionTranches, TrancheEssence, TrancheInput, TrancheSolution, TrancheUpdate, Tranches, }; // The TypeId impl we derive pool-accounts from @@ -240,7 +240,15 @@ impl< .map_err(Into::into) } - pub fn essence( + /// Constructs the pool essence from tranche metadata stored in the asset + /// registry. + /// + /// Must not be used for the creation of a new pool as each tranche token is + /// required to be registered in the registry which happens after the pool + /// creation for Subquery convenience. + /// + /// NOTE: Uses `essence_from_tranche_input` under the hood. + pub fn essence_from_registry( &self, ) -> Result, DispatchError> where @@ -248,7 +256,8 @@ impl< orml_traits::asset_registry::Inspect, StringLimit: Get, { - let mut tranches: Vec> = Vec::new(); + let mut ids: Vec = Vec::new(); + let mut tranche_input: Vec> = Vec::new(); for tranche in self.tranches.residual_top_slice().iter() { let metadata = AssetRegistry::metadata( @@ -258,9 +267,10 @@ impl< ) .ok_or(DispatchError::CannotLookup)?; - tranches.push(TrancheEssence { - currency: tranche.currency, - ty: tranche.tranche_type, + ids.push(tranche.currency); + tranche_input.push(TrancheInput { + tranche_type: tranche.tranche_type, + seniority: Some(tranche.seniority), metadata: TrancheMetadata { token_name: metadata.name, token_symbol: metadata.symbol, @@ -268,12 +278,42 @@ impl< }); } + Self::essence_from_tranche_input::(self, ids, tranche_input) + } + + /// Constructs the pool essence from the input tranche metadata such that. + /// + /// Does not require each tranche token to be registered in some storage + /// such that this function can be called when a new pool is created. + pub fn essence_from_tranche_input( + &self, + ids: Vec, + tranche_input: Vec>, + ) -> Result, DispatchError> + where + StringLimit: Get, + TrancheCurrency: Into, + { + let mut tranche_essence: Vec> = + Vec::new(); + + for (id, input) in ids.into_iter().zip(tranche_input) { + tranche_essence.push(TrancheEssence { + currency: id, + ty: input.tranche_type, + metadata: TrancheMetadata { + token_name: input.metadata.token_name, + token_symbol: input.metadata.token_symbol, + }, + }); + } + Ok(PoolEssence { currency: self.currency, max_reserve: self.reserve.max, max_nav_age: self.parameters.max_nav_age, min_epoch_time: self.parameters.min_epoch_time, - tranches, + tranches: tranche_essence, }) } } diff --git a/pallets/pool-system/src/tests/mod.rs b/pallets/pool-system/src/tests/mod.rs index b9bcb2d916..6f25a25a24 100644 --- a/pallets/pool-system/src/tests/mod.rs +++ b/pallets/pool-system/src/tests/mod.rs @@ -2478,7 +2478,7 @@ fn essence() { let pool_details = Pool::::get(DEFAULT_POOL_ID).expect("Pool is registered; qed"); let essence = pool_details - .essence::<::AssetRegistry, Balance, StringLimit>() + .essence_from_registry::<::AssetRegistry, Balance, StringLimit>() .expect("Tranche token metadata is registered; qed"); assert_eq!(essence.currency, AUSD_CURRENCY_ID); @@ -2787,8 +2787,26 @@ mod pool_fees { ], AUSD_CURRENCY_ID, DEFAULT_POOL_MAX_RESERVE, - fees, + fees.clone(), )); + + if !fees.is_empty() { + let pos_pool_creation = System::events() + .iter() + .position(|e| match e.event { + RuntimeEvent::PoolSystem(Event::Created { .. }) => true, + _ => false, + }) + .expect("Pool created; qed"); + let pos_pool_fee_added = System::events() + .iter() + .position(|e| match e.event { + RuntimeEvent::PoolFees(pallet_pool_fees::Event::Added { .. }) => true, + _ => false, + }) + .expect("Pool fees added; qed"); + assert!(pos_pool_creation < pos_pool_fee_added); + } test_nav_up(DEFAULT_POOL_ID, NAV_AMOUNT); // Force min_epoch_time to 0 without using update