fix: fix inconsistencies in node contracts states

Author: sameh-farouk

substrate-node/pallets/pallet-smart-contract/src/billing.rs

@@ -362,8 +362,14 @@ impl<T: Config> Pallet<T> {
         has_sufficient_fund: bool,
         current_block: u64,
     ) -> DispatchResultWithPostInfo {
+        // Add a defensive check to prevent a node contract on a rented node from transitioning to 'created' status without considering the state of the associated rent contract
+        let rent_is_suspended = matches!(contract.contract_type, types::ContractData::NodeContract(_))
+        && ActiveRentContractForNode::<T>::get(contract.get_node_id())
+            .and_then(|id| Contracts::<T>::get(&id))
+            .map(|c| matches!(c.state, types::ContractState::GracePeriod(_)))
+            .unwrap_or(false);
         match contract.state {
-            types::ContractState::GracePeriod(_) if has_sufficient_fund => {
+            types::ContractState::GracePeriod(_) if has_sufficient_fund && !rent_is_suspended => {
                 // Manage transition from GracePeriod to Created
                 log::info!("Contract {:?} is in grace period, but balance is recharged, moving to created state at block {:?}", contract.contract_id, current_block);
                 Self::update_contract_state(contract, &types::ContractState::Created)?;
@@ -386,6 +392,26 @@ impl<T: Config> Pallet<T> {
                     diff
                 );
                 if diff >= T::GracePeriod::get() {
+                    // Ensure associated node contracts have no reserved balance.
+                    // If none, proceed to move the contract to the 'deleted' state.
+                    // Otherwise, wait.
+                    let can_be_deleted = match contract.contract_type {
+                        types::ContractData::RentContract(_) => {
+                            let active_node_contracts = ActiveNodeContracts::<T>::get(contract.get_node_id());
+                            !active_node_contracts.iter().any(|id| {
+                                ContractPaymentState::<T>::get(id).map_or(false, |s| s.has_reserve())
+                            })
+                        }
+                        _ => true,
+                    };
+                    if !can_be_deleted {
+                        log::debug!(
+                            "Grace period expired, but one or more associated node contracts have held user funds. \
+                            Rent contract deletion will be delayed until funds are distributed to beneficiaries."
+                        );
+                        return Ok(().into());
+                    }
+
                     log::info!("Contract {:?} state changed to deleted at block {:?} due to an expired grace period.", contract.contract_id, current_block);
                     Self::deposit_event(Event::ContractGracePeriodElapsed {
                         contract_id: contract.contract_id,
@@ -734,6 +760,14 @@ impl<T: Config> Pallet<T> {
                 for ctr_id in active_node_contracts {
                     let mut ctr =
                         Contracts::<T>::get(ctr_id).ok_or(Error::<T>::ContractNotExists)?;
+
+                    // Defensive check to prevent a node contract on a rented node from transitioning to 'created' status if there's an overdraft (unsettled public IP rent).
+                    if ContractPaymentState::<T>::get(ctr_id).map_or_else(|| false, |ps| ps.has_overdraft())
+                        && matches!(state, types::ContractState::Created)
+                    {
+                        continue;
+                    }
+ 
                     Self::update_contract_state(&mut ctr, &state)?;
 
                     match state {

substrate-node/pallets/pallet-smart-contract/src/grid_contract.rs

@@ -270,9 +270,12 @@ impl<T: Config> Pallet<T> {
         // override values
         contract.contract_type = types::ContractData::NodeContract(node_contract);
 
-        let state = contract.state.clone();
-        Self::update_contract_state(&mut contract, &state)?;
-
+        // Why we call update_contract_state if state can't be changed?
+        // Obviously, we just depened on the update_contract_state for writing the contract to the storage, but we can do it here
+        // let state = contract.state.clone();
+        // Self::update_contract_state(&mut contract, &state)?;
+        Contracts::<T>::insert(&contract.contract_id, contract.clone());
+        
         Self::deposit_event(Event::ContractUpdated(contract));
 
         Ok(().into())
@@ -391,24 +394,35 @@ impl<T: Config> Pallet<T> {
     fn remove_active_node_contract(node_id: u32, contract_id: u64) {
         let mut contracts = ActiveNodeContracts::<T>::get(&node_id);
 
-        match contracts.iter().position(|id| id == &contract_id) {
-            Some(index) => {
-                contracts.remove(index);
-            }
-            None => (),
-        };
+        let initial_len = contracts.len();
+
+        contracts.retain(|&id| id != contract_id);
 
-        ActiveNodeContracts::<T>::insert(&node_id, &contracts);
+        // Only update the storage if the list length has changed, meaning a contract was removed
+        if contracts.len() != initial_len {
+            ActiveNodeContracts::<T>::insert(&node_id, contracts);
+        }
     }
 
     // Helper function that updates the contract state and manages storage accordingly
     pub fn update_contract_state(
         contract: &mut types::Contract<T>,
         state: &types::ContractState,
     ) -> DispatchResultWithPostInfo {
-        // update the state and save the contract
-        contract.state = state.clone();
-        Contracts::<T>::insert(&contract.contract_id, contract.clone());
+        // Update the state and save the contract only when necessary.
+        if contract.state != *state {
+            match (&contract.state, state) {
+                // Transition from GracePeriod(x) to GracePeriod(y) is not allowed
+                (types::ContractState::GracePeriod(_), types::ContractState::GracePeriod(_)) => {
+                    log::info!("Contract {:?} already in grace period, not changing to {:?} state", contract.contract_id, state);
+                },
+                // All other transitions are allowed
+                _ => {
+                    contract.state = state.clone();
+                    Contracts::<T>::insert(&contract.contract_id, contract.clone());
+                }
+            }
+         }
 
         // if the contract is a name or rent contract, nothing to do left here
         match contract.contract_type {
@@ -423,6 +437,7 @@ impl<T: Config> Pallet<T> {
 
         let mut contracts = ActiveNodeContracts::<T>::get(&node_contract.node_id);
 
+        let original_len = contracts.len();
         match contracts.iter().position(|id| id == &contract.contract_id) {
             Some(index) => {
                 // if the new contract state is delete, remove the contract id from the map
@@ -437,8 +452,9 @@ impl<T: Config> Pallet<T> {
                 }
             }
         };
-
-        ActiveNodeContracts::<T>::insert(&node_contract.node_id, &contracts);
+        if contracts.len() != original_len {
+            ActiveNodeContracts::<T>::insert(&node_contract.node_id, &contracts);
+        };
 
         Ok(().into())
     }