Integrated dApps improvement (#1153)

* Integrated dApps improvement

* Additional UT

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::<T>::get().era;
-
-            let mut dapp_info =
+            let dapp_info =
                 IntegratedDApps::<T>::get(&smart_contract).ok_or(Error::<T>::ContractNotFound)?;
 
-            ensure!(dapp_info.is_registered(), Error::<T>::NotOperatedDApp);
-
             ContractStake::<T>::remove(&dapp_info.id);
+            IntegratedDApps::<T>::remove(&smart_contract);
 
-            dapp_info.state = DAppState::Unregistered(current_era);
-            IntegratedDApps::<T>::insert(&smart_contract, dapp_info);
-
+            let current_era = ActiveProtocolState::<T>::get().era;
             Self::deposit_event(Event::<T>::DAppUnregistered {
                 smart_contract,
                 era: current_era,
@@ -944,8 +936,7 @@ pub mod pallet {
             ensure!(amount > 0, Error::<T>::ZeroAmount);
 
             let dapp_info =
-                IntegratedDApps::<T>::get(&smart_contract).ok_or(Error::<T>::NotOperatedDApp)?;
-            ensure!(dapp_info.is_registered(), Error::<T>::NotOperatedDApp);
+                IntegratedDApps::<T>::get(&smart_contract).ok_or(Error::<T>::ContractNotFound)?;
 
             let protocol_state = ActiveProtocolState::<T>::get();
             let current_era = protocol_state.era;
@@ -1071,8 +1062,7 @@ pub mod pallet {
             ensure!(amount > 0, Error::<T>::ZeroAmount);
 
             let dapp_info =
-                IntegratedDApps::<T>::get(&smart_contract).ok_or(Error::<T>::NotOperatedDApp)?;
-            ensure!(dapp_info.is_registered(), Error::<T>::NotOperatedDApp);
+                IntegratedDApps::<T>::get(&smart_contract).ok_or(Error::<T>::ContractNotFound)?;
 
             let protocol_state = ActiveProtocolState::<T>::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::<T>::contains_key(&smart_contract),
                 Error::<T>::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::<T>::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())

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<TierId>,
+/// 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<MD: Get<u32>, NT: Get<u32>> {
-    /// DApps and their corresponding tiers (or `None` if they have been claimed in the meantime)
-    pub dapps: BoundedVec<OldDAppTier, MD>,
-    /// Rewards for each tier. First entry refers to the first tier, and so on.
-    pub rewards: BoundedVec<Balance, NT>,
-    /// 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<AccountId> {
+    /// Owner of the dApp, default reward beneficiary.
+    pub owner: AccountId,
+    /// dApp's unique identifier in dApp staking.
     #[codec(compact)]
-    pub period: PeriodNumber,
-}
-
-impl<MD: Get<u32>, NT: Get<u32>> Default for OldDAppTierRewards<MD, NT> {
-    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<AccountId>,
 }
 
-// Legacy convenience type for `DAppTierRewards` usage.
-type OldDAppTierRewardsFor<T> =
-    OldDAppTierRewards<<T as Config>::MaxNumberOfContracts, <T as Config>::NumberOfTiers>;
-
-/// `OnRuntimeUpgrade` logic used to migrate DApp tiers storage item to BTreeMap.
-pub struct DappStakingV3TierRewardAsTree<T>(PhantomData<T>);
-impl<T: Config> OnRuntimeUpgrade for DappStakingV3TierRewardAsTree<T> {
+/// To be only used for Shibuya, can be removed later.
+pub struct DAppStakingV3IntegratedDAppsMigration<T>(PhantomData<T>);
+impl<T: Config> OnRuntimeUpgrade for DAppStakingV3IntegratedDAppsMigration<T> {
     fn on_runtime_upgrade() -> Weight {
-        let mut counter = 0;
-        let mut translate = |pre: OldDAppTierRewardsFor<T>| -> DAppTierRewardsFor<T> {
-            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::<T>::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::<T>::translate::<OldDAppInfo<T::AccountId>, _>(|_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::<T>::new(BTreeMap::new(), pre.rewards.to_vec(), pre.period)
-                    .unwrap_or_default(),
-            )
-        };
-
-        DAppTiers::<T>::translate(|_key, value: OldDAppTierRewardsFor<T>| 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<T>(PhantomData<T>);
-impl<T: Config> OnRuntimeUpgrade for DappStakingV3HistoryCleanupMarkerReset<T> {
-    fn on_runtime_upgrade() -> Weight {
-        HistoryCleanupMarker::<T>::put(CleanupMarker::default());
-        T::DbWeight::get().writes(1)
+        T::DbWeight::get().reads_writes(translated, translated + 1 /* counted map */)
     }
 }

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::<Test>::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::<Test>::contains_key(&smart_contract));
     assert_eq!(
-        IntegratedDApps::<Test>::get(&smart_contract).unwrap().state,
-        DAppState::Unregistered(pre_snapshot.active_protocol_state.era),
+        pre_snapshot.integrated_dapps.len() - 1,
+        IntegratedDApps::<Test>::count() as usize
     );
+
     assert!(!ContractStake::<Test>::contains_key(
-        &IntegratedDApps::<Test>::get(&smart_contract).unwrap().id
+        &pre_snapshot.integrated_dapps[&smart_contract].id
     ));
 }
 

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::<Test>::NotOperatedDApp
+            Error::<Test>::ContractNotFound
         );
     })
 }
@@ -961,15 +961,15 @@ 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::<Test>::NotOperatedDApp
+            Error::<Test>::ContractNotFound
         );
 
         // Try to stake on unregistered smart contract
         assert_register(1, &smart_contract);
         assert_unregister(&smart_contract);
         assert_noop!(
             DappStaking::stake(RuntimeOrigin::signed(account), smart_contract, 100),
-            Error::<Test>::NotOperatedDApp
+            Error::<Test>::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::<Test>::NotOperatedDApp
+            Error::<Test>::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::<Test>::NotOperatedDApp
+            Error::<Test>::ContractNotFound
         );
     })
 }
@@ -2688,3 +2688,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 = <Test as Config>::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::<Test>::ExceededMaxNumberOfContracts
+        );
+
+        // Unregister one contract, and ensure register works again
+        let smart_contract = MockSmartContract::Wasm(0);
+        assert_unregister(&smart_contract);
+        assert_register(developer, &smart_contract);
+    })
+}

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]

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<AccountId> {
@@ -264,8 +255,6 @@ pub struct DAppInfo<AccountId> {
     /// 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<AccountId>,
 }
@@ -278,11 +267,6 @@ impl<AccountId> DAppInfo<AccountId> {
             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.