From e009c37bae9359ee561f9a5882642cfc2ff6c51c Mon Sep 17 00:00:00 2001 From: Dino Pacandi Date: Tue, 23 Jan 2024 18:33:43 +0100 Subject: [PATCH 1/2] Integrated dApps improvement --- pallets/dapp-staking-v3/src/lib.rs | 30 ++--- pallets/dapp-staking-v3/src/migrations.rs | 113 ++++++------------ .../dapp-staking-v3/src/test/testing_utils.rs | 9 +- pallets/dapp-staking-v3/src/test/tests.rs | 10 +- .../dapp-staking-v3/src/test/tests_types.rs | 7 -- pallets/dapp-staking-v3/src/types.rs | 16 --- pallets/inflation/src/lib.rs | 21 ---- runtime/shibuya/src/lib.rs | 77 +----------- 8 files changed, 53 insertions(+), 230 deletions(-) diff --git a/pallets/dapp-staking-v3/src/lib.rs b/pallets/dapp-staking-v3/src/lib.rs index e1854b0e5d..8bf1e0d36e 100644 --- a/pallets/dapp-staking-v3/src/lib.rs +++ b/pallets/dapp-staking-v3/src/lib.rs @@ -303,8 +303,6 @@ pub mod pallet { ContractNotFound, /// Call origin is not dApp owner. OriginNotOwner, - /// dApp is part of dApp staking but isn't active anymore. - NotOperatedDApp, /// Performing locking or staking with 0 amount. ZeroAmount, /// Total locked amount for staker is below minimum threshold. @@ -637,7 +635,6 @@ pub mod pallet { DAppInfo { owner: owner.clone(), id: dapp_id, - state: DAppState::Registered, reward_beneficiary: None, }, ); @@ -746,18 +743,13 @@ pub mod pallet { Self::ensure_pallet_enabled()?; T::ManagerOrigin::ensure_origin(origin)?; - let current_era = ActiveProtocolState::::get().era; - - let mut dapp_info = + let dapp_info = IntegratedDApps::::get(&smart_contract).ok_or(Error::::ContractNotFound)?; - ensure!(dapp_info.is_registered(), Error::::NotOperatedDApp); - ContractStake::::remove(&dapp_info.id); + IntegratedDApps::::remove(&smart_contract); - dapp_info.state = DAppState::Unregistered(current_era); - IntegratedDApps::::insert(&smart_contract, dapp_info); - + let current_era = ActiveProtocolState::::get().era; Self::deposit_event(Event::::DAppUnregistered { smart_contract, era: current_era, @@ -944,8 +936,7 @@ pub mod pallet { ensure!(amount > 0, Error::::ZeroAmount); let dapp_info = - IntegratedDApps::::get(&smart_contract).ok_or(Error::::NotOperatedDApp)?; - ensure!(dapp_info.is_registered(), Error::::NotOperatedDApp); + IntegratedDApps::::get(&smart_contract).ok_or(Error::::ContractNotFound)?; let protocol_state = ActiveProtocolState::::get(); let current_era = protocol_state.era; @@ -1071,8 +1062,7 @@ pub mod pallet { ensure!(amount > 0, Error::::ZeroAmount); let dapp_info = - IntegratedDApps::::get(&smart_contract).ok_or(Error::::NotOperatedDApp)?; - ensure!(dapp_info.is_registered(), Error::::NotOperatedDApp); + IntegratedDApps::::get(&smart_contract).ok_or(Error::::ContractNotFound)?; let protocol_state = ActiveProtocolState::::get(); let current_era = protocol_state.era; @@ -1159,7 +1149,7 @@ pub mod pallet { } /// Claims some staker rewards, if user has any. - /// In the case of a successfull call, at least one era will be claimed, with the possibility of multiple claims happening. + /// In the case of a successful call, at least one era will be claimed, with the possibility of multiple claims happening. #[pallet::call_index(13)] #[pallet::weight({ let max_span_length = T::EraRewardSpanLength::get(); @@ -1384,7 +1374,7 @@ pub mod pallet { let account = ensure_signed(origin)?; ensure!( - !Self::is_registered(&smart_contract), + !IntegratedDApps::::contains_key(&smart_contract), Error::::ContractStillActive ); @@ -1620,12 +1610,6 @@ pub mod pallet { .saturating_mul(T::CycleConfiguration::eras_per_voting_subperiod().into()) } - /// `true` if smart contract is registered, `false` otherwise. - pub(crate) fn is_registered(smart_contract: &T::SmartContract) -> bool { - IntegratedDApps::::get(smart_contract) - .map_or(false, |dapp_info| dapp_info.is_registered()) - } - /// Calculates the `EraRewardSpan` index for the specified era. pub fn era_reward_span_index(era: EraNumber) -> EraNumber { era.saturating_sub(era % T::EraRewardSpanLength::get()) diff --git a/pallets/dapp-staking-v3/src/migrations.rs b/pallets/dapp-staking-v3/src/migrations.rs index 52e5f6da74..300b580688 100644 --- a/pallets/dapp-staking-v3/src/migrations.rs +++ b/pallets/dapp-staking-v3/src/migrations.rs @@ -121,92 +121,47 @@ impl< } } -/// Legacy struct type -/// Should be deleted after the migration -#[derive(Encode, Decode, MaxEncodedLen, Copy, Clone, Debug, PartialEq, Eq, TypeInfo)] -struct OldDAppTier { - #[codec(compact)] - pub dapp_id: DAppId, - pub tier_id: Option, +/// State in which some dApp is in. +#[derive(Encode, Decode, MaxEncodedLen, Clone, Copy, Debug, PartialEq, Eq, TypeInfo)] +pub enum OldDAppState { + /// dApp is registered and active. + Registered, + /// dApp has been unregistered in the contained era. + Unregistered(#[codec(compact)] EraNumber), } -/// Information about all of the dApps that got into tiers, and tier rewards -#[derive( - Encode, - Decode, - MaxEncodedLen, - RuntimeDebugNoBound, - PartialEqNoBound, - EqNoBound, - CloneNoBound, - TypeInfo, -)] -#[scale_info(skip_type_params(MD, NT))] -struct OldDAppTierRewards, NT: Get> { - /// DApps and their corresponding tiers (or `None` if they have been claimed in the meantime) - pub dapps: BoundedVec, - /// Rewards for each tier. First entry refers to the first tier, and so on. - pub rewards: BoundedVec, - /// Period during which this struct was created. +/// General information about a dApp. +#[derive(Encode, Decode, MaxEncodedLen, Clone, Copy, Debug, PartialEq, Eq, TypeInfo)] +pub struct OldDAppInfo { + /// Owner of the dApp, default reward beneficiary. + pub owner: AccountId, + /// dApp's unique identifier in dApp staking. #[codec(compact)] - pub period: PeriodNumber, -} - -impl, NT: Get> Default for OldDAppTierRewards { - fn default() -> Self { - Self { - dapps: BoundedVec::default(), - rewards: BoundedVec::default(), - period: 0, - } - } + pub id: DAppId, + /// Current state of the dApp. + pub state: OldDAppState, + // If `None`, rewards goes to the developer account, otherwise to the account Id in `Some`. + pub reward_beneficiary: Option, } -// Legacy convenience type for `DAppTierRewards` usage. -type OldDAppTierRewardsFor = - OldDAppTierRewards<::MaxNumberOfContracts, ::NumberOfTiers>; - -/// `OnRuntimeUpgrade` logic used to migrate DApp tiers storage item to BTreeMap. -pub struct DappStakingV3TierRewardAsTree(PhantomData); -impl OnRuntimeUpgrade for DappStakingV3TierRewardAsTree { +/// To be only used for Shibuya, can be removed later. +pub struct DAppStakingV3IntegratedDAppsMigration(PhantomData); +impl OnRuntimeUpgrade for DAppStakingV3IntegratedDAppsMigration { fn on_runtime_upgrade() -> Weight { - let mut counter = 0; - let mut translate = |pre: OldDAppTierRewardsFor| -> DAppTierRewardsFor { - let mut dapps_tree = BTreeMap::new(); - for dapp_tier in &pre.dapps { - if let Some(tier_id) = dapp_tier.tier_id { - dapps_tree.insert(dapp_tier.dapp_id, tier_id); - } - } - - let result = DAppTierRewardsFor::::new(dapps_tree, pre.rewards.to_vec(), pre.period); - if result.is_err() { - // Tests should ensure this never happens... - log::error!("Failed to migrate dApp tier rewards: {:?}", pre); + let mut translated = 0_u64; + IntegratedDApps::::translate::, _>(|_key, old_value| { + translated.saturating_inc(); + + match old_value.state { + OldDAppState::Registered => Some(DAppInfo { + owner: old_value.owner, + id: old_value.id, + reward_beneficiary: old_value.reward_beneficiary, + }), + OldDAppState::Unregistered(_) => None, } + }); - // For weight calculation purposes - counter.saturating_inc(); - - // ...if it does happen, there's not much to do except create an empty map - result.unwrap_or( - DAppTierRewardsFor::::new(BTreeMap::new(), pre.rewards.to_vec(), pre.period) - .unwrap_or_default(), - ) - }; - - DAppTiers::::translate(|_key, value: OldDAppTierRewardsFor| Some(translate(value))); - - T::DbWeight::get().reads_writes(counter, counter) - } -} - -/// We just set it to default value (all zeros) and let the pallet itself do the history cleanup. -/// Only needed for Shibuya, can be removed later. -pub struct DappStakingV3HistoryCleanupMarkerReset(PhantomData); -impl OnRuntimeUpgrade for DappStakingV3HistoryCleanupMarkerReset { - fn on_runtime_upgrade() -> Weight { - HistoryCleanupMarker::::put(CleanupMarker::default()); - T::DbWeight::get().writes(1) + T::DbWeight::get().reads_writes(translated, translated + 1 /* counted map */) } } diff --git a/pallets/dapp-staking-v3/src/test/testing_utils.rs b/pallets/dapp-staking-v3/src/test/testing_utils.rs index b6ed9e3511..6c4cf6f9d9 100644 --- a/pallets/dapp-staking-v3/src/test/testing_utils.rs +++ b/pallets/dapp-staking-v3/src/test/testing_utils.rs @@ -112,7 +112,6 @@ pub(crate) fn assert_register(owner: AccountId, smart_contract: &MockSmartContra // Verify post-state let dapp_info = IntegratedDApps::::get(smart_contract).unwrap(); - assert_eq!(dapp_info.state, DAppState::Registered); assert_eq!(dapp_info.owner, owner); assert_eq!(dapp_info.id, pre_snapshot.next_dapp_id); assert!(dapp_info.reward_beneficiary.is_none()); @@ -194,12 +193,14 @@ pub(crate) fn assert_unregister(smart_contract: &MockSmartContract) { })); // Verify post-state + assert!(!IntegratedDApps::::contains_key(&smart_contract)); assert_eq!( - IntegratedDApps::::get(&smart_contract).unwrap().state, - DAppState::Unregistered(pre_snapshot.active_protocol_state.era), + pre_snapshot.integrated_dapps.len() - 1, + IntegratedDApps::::count() as usize ); + assert!(!ContractStake::::contains_key( - &IntegratedDApps::::get(&smart_contract).unwrap().id + &pre_snapshot.integrated_dapps[&smart_contract].id )); } diff --git a/pallets/dapp-staking-v3/src/test/tests.rs b/pallets/dapp-staking-v3/src/test/tests.rs index 3e409379a7..c4859ea7b3 100644 --- a/pallets/dapp-staking-v3/src/test/tests.rs +++ b/pallets/dapp-staking-v3/src/test/tests.rs @@ -463,7 +463,7 @@ fn unregister_fails() { assert_unregister(&smart_contract); assert_noop!( DappStaking::unregister(RuntimeOrigin::root(), smart_contract), - Error::::NotOperatedDApp + Error::::ContractNotFound ); }) } @@ -961,7 +961,7 @@ fn stake_on_invalid_dapp_fails() { let smart_contract = MockSmartContract::wasm(1 as AccountId); assert_noop!( DappStaking::stake(RuntimeOrigin::signed(account), smart_contract, 100), - Error::::NotOperatedDApp + Error::::ContractNotFound ); // Try to stake on unregistered smart contract @@ -969,7 +969,7 @@ fn stake_on_invalid_dapp_fails() { assert_unregister(&smart_contract); assert_noop!( DappStaking::stake(RuntimeOrigin::signed(account), smart_contract, 100), - Error::::NotOperatedDApp + Error::::ContractNotFound ); }) } @@ -1237,7 +1237,7 @@ fn unstake_on_invalid_dapp_fails() { let smart_contract = MockSmartContract::wasm(1 as AccountId); assert_noop!( DappStaking::unstake(RuntimeOrigin::signed(account), smart_contract, 100), - Error::::NotOperatedDApp + Error::::ContractNotFound ); // Try to unstake from unregistered smart contract @@ -1246,7 +1246,7 @@ fn unstake_on_invalid_dapp_fails() { assert_unregister(&smart_contract); assert_noop!( DappStaking::unstake(RuntimeOrigin::signed(account), smart_contract, 100), - Error::::NotOperatedDApp + Error::::ContractNotFound ); }) } diff --git a/pallets/dapp-staking-v3/src/test/tests_types.rs b/pallets/dapp-staking-v3/src/test/tests_types.rs index 5e5eebf5ef..64da02a23e 100644 --- a/pallets/dapp-staking-v3/src/test/tests_types.rs +++ b/pallets/dapp-staking-v3/src/test/tests_types.rs @@ -157,7 +157,6 @@ fn dapp_info_basic_checks() { let mut dapp_info = DAppInfo { owner, id: 7, - state: DAppState::Registered, reward_beneficiary: None, }; @@ -167,12 +166,6 @@ fn dapp_info_basic_checks() { // Beneficiary receives rewards in case it is set dapp_info.reward_beneficiary = Some(beneficiary); assert_eq!(*dapp_info.reward_beneficiary(), beneficiary); - - // Check if dApp is registered - assert!(dapp_info.is_registered()); - - dapp_info.state = DAppState::Unregistered(10); - assert!(!dapp_info.is_registered()); } #[test] diff --git a/pallets/dapp-staking-v3/src/types.rs b/pallets/dapp-staking-v3/src/types.rs index 870e00223f..4101fa1990 100644 --- a/pallets/dapp-staking-v3/src/types.rs +++ b/pallets/dapp-staking-v3/src/types.rs @@ -247,15 +247,6 @@ impl ProtocolState { } } -/// State in which some dApp is in. -#[derive(Encode, Decode, MaxEncodedLen, Clone, Copy, Debug, PartialEq, Eq, TypeInfo)] -pub enum DAppState { - /// dApp is registered and active. - Registered, - /// dApp has been unregistered in the contained era. - Unregistered(#[codec(compact)] EraNumber), -} - /// General information about a dApp. #[derive(Encode, Decode, MaxEncodedLen, Clone, Copy, Debug, PartialEq, Eq, TypeInfo)] pub struct DAppInfo { @@ -264,8 +255,6 @@ pub struct DAppInfo { /// dApp's unique identifier in dApp staking. #[codec(compact)] pub id: DAppId, - /// Current state of the dApp. - pub state: DAppState, // If `None`, rewards goes to the developer account, otherwise to the account Id in `Some`. pub reward_beneficiary: Option, } @@ -278,11 +267,6 @@ impl DAppInfo { None => &self.owner, } } - - /// `true` if dApp is registered, `false` otherwise. - pub fn is_registered(&self) -> bool { - self.state == DAppState::Registered - } } /// How much was unlocked in some block. diff --git a/pallets/inflation/src/lib.rs b/pallets/inflation/src/lib.rs index bfbf364bc7..303472ac96 100644 --- a/pallets/inflation/src/lib.rs +++ b/pallets/inflation/src/lib.rs @@ -671,24 +671,3 @@ impl> OnRuntimeUpgrade Ok(()) } } - -/// To be used just for Shibuya, can be removed after migration has been executed. -pub struct PalletInflationShibuyaMigration(PhantomData<(T, P)>); -impl> OnRuntimeUpgrade - for PalletInflationShibuyaMigration -{ - fn on_runtime_upgrade() -> Weight { - let (recalculation_era, weight) = P::get(); - ActiveInflationConfig::::mutate(|config| { - // Both recalculation_era and recalculation_block are of the same `u32` type so no need to do any translation. - config.recalculation_era = recalculation_era; - }); - - log::info!( - "Inflation pallet recalculation era set to {}.", - recalculation_era - ); - - T::DbWeight::get().reads_writes(1, 1).saturating_add(weight) - } -} diff --git a/runtime/shibuya/src/lib.rs b/runtime/shibuya/src/lib.rs index c6e969308b..aa49e31f7f 100644 --- a/runtime/shibuya/src/lib.rs +++ b/runtime/shibuya/src/lib.rs @@ -53,7 +53,6 @@ use pallet_transaction_payment::{ use parity_scale_codec::{Compact, Decode, Encode, MaxEncodedLen}; use polkadot_runtime_common::BlockHashCount; use sp_api::impl_runtime_apis; -use sp_arithmetic::fixed_point::FixedU64; use sp_core::{ConstBool, OpaqueMetadata, H160, H256, U256}; use sp_inherents::{CheckInherentsResult, InherentData}; use sp_runtime::{ @@ -1340,80 +1339,8 @@ pub type Executive = frame_executive::Executive< /// All migrations that will run on the next runtime upgrade. /// /// Once done, migrations should be removed from the tuple. -pub struct InitActivePriceGet; -impl Get for InitActivePriceGet { - fn get() -> FixedU64 { - FixedU64::from_rational(1, 10) - } -} - -parameter_types! { - pub const DappsStakingV2Name: &'static str = "DappsStaking"; - pub const DappStakingMigrationName: &'static str = "DappStakingMigration"; -} - -pub type Migrations = ( - pallet_static_price_provider::InitActivePrice, - pallet_dapp_staking_v3::migrations::DappStakingV3TierRewardAsTree, - pallet_dapp_staking_v3::migrations::DappStakingV3HistoryCleanupMarkerReset, - pallet_inflation::PalletInflationShibuyaMigration, - frame_support::migrations::RemovePallet< - DappsStakingV2Name, - ::DbWeight, - >, - frame_support::migrations::RemovePallet< - DappStakingMigrationName, - ::DbWeight, - >, -); - -pub struct NextEraProvider; -impl Get<(EraNumber, Weight)> for NextEraProvider { - fn get() -> (EraNumber, Weight) { - // Prior to executing the migration, `recalculation_era` is still set to the old `recalculation_block` - let target_block = - pallet_inflation::ActiveInflationConfig::::get().recalculation_era; - - let state = pallet_dapp_staking_v3::ActiveProtocolState::::get(); - - // Best case scenario, the target era is the first era of the next period. - use pallet_dapp_staking_v3::Subperiod; - let mut target_era = match state.subperiod() { - Subperiod::Voting => state.era.saturating_add( - InflationCycleConfig::eras_per_build_and_earn_subperiod().saturating_add(1), - ), - Subperiod::BuildAndEarn => state.next_subperiod_start_era(), - }; - - // Adding the whole period length in blocks to the current block number, and comparing it with the target - // is good enough to find the target era. - let period_length_in_blocks = InflationCycleConfig::blocks_per_cycle() - / InflationCycleConfig::periods_per_cycle().max(1); - let mut block = System::block_number().saturating_add(period_length_in_blocks); - - // Max number of iterations is the number of periods per cycle, it's not possible for more than that to occur. - let mut limit = InflationCycleConfig::periods_per_cycle(); - - use sp_runtime::traits::Saturating; - while block < target_block && limit > 0 { - target_era.saturating_accrue(InflationCycleConfig::eras_per_period()); - block.saturating_accrue(period_length_in_blocks); - - limit.saturating_dec() - } - - #[cfg(feature = "try-runtime")] - if block < target_block { - panic!("Failed to find target era for migration, please check for errors"); - } - - // A bit overestimated weight, but it's fine since we have some calculations to execute in this function which consume some time. - ( - target_era, - ::DbWeight::get().reads(3), - ) - } -} +pub type Migrations = + (pallet_dapp_staking_v3::migrations::DAppStakingV3IntegratedDAppsMigration,); type EventRecord = frame_system::EventRecord< ::RuntimeEvent, From 7d39da3336953f74da928f8f34f5eef01fc9de00 Mon Sep 17 00:00:00 2001 From: Dino Pacandi Date: Wed, 24 Jan 2024 08:44:46 +0100 Subject: [PATCH 2/2] Additional UT --- pallets/dapp-staking-v3/src/test/tests.rs | 28 +++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/pallets/dapp-staking-v3/src/test/tests.rs b/pallets/dapp-staking-v3/src/test/tests.rs index c4859ea7b3..afda33d7a9 100644 --- a/pallets/dapp-staking-v3/src/test/tests.rs +++ b/pallets/dapp-staking-v3/src/test/tests.rs @@ -2671,3 +2671,31 @@ fn observer_pre_new_era_block_works() { assert_observer_value(4); }) } + +#[test] +fn unregister_after_max_number_of_contracts_allows_register_again() { + ExtBuilder::build().execute_with(|| { + let max_number_of_contracts = ::MaxNumberOfContracts::get(); + let developer = 2; + + // Reach max number of contracts + for id in 0..max_number_of_contracts { + assert_register(developer, &MockSmartContract::Wasm(id.into())); + } + + // Ensure we cannot register more contracts + assert_noop!( + DappStaking::register( + RuntimeOrigin::root(), + developer, + MockSmartContract::Wasm((max_number_of_contracts).into()) + ), + Error::::ExceededMaxNumberOfContracts + ); + + // Unregister one contract, and ensure register works again + let smart_contract = MockSmartContract::Wasm(0); + assert_unregister(&smart_contract); + assert_register(developer, &smart_contract); + }) +}