feat: rework contract billing storage (#477)

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

@@ -25,7 +25,6 @@ use pallet_tfgrid::types as pallet_tfgrid_types;
 use pallet_timestamp as timestamp;
 use sp_core::crypto::KeyTypeId;
 use sp_runtime::{
-    offchain::storage::StorageValueRef,
     traits::{CheckedSub, SaturatedConversion},
     Perbill,
 };
@@ -77,6 +76,7 @@ pub mod crypto {
 pub mod weights;
 
 pub mod cost;
+pub mod migration;
 pub mod name_contract;
 pub mod types;
 pub mod migrations;
@@ -487,10 +487,9 @@ pub mod pallet {
         pub fn bill_contract_for_block(
             origin: OriginFor<T>,
             contract_id: u64,
-            block_number: T::BlockNumber,
         ) -> DispatchResultWithPostInfo {
             let _account_id = ensure_signed(origin)?;
-            Self::_bill_contract_for_block(contract_id, block_number)
+            Self::_bill_contract(contract_id)
         }
     }
 
@@ -501,14 +500,6 @@ pub mod pallet {
             // Index being current block number % (mod) Billing Frequency
             let current_index: u64 =
                 block_number.saturated_into::<u64>() % BillingFrequency::<T>::get();
-            //reset the failed contract ids for next run
-            Self::save_failed_contract_ids_in_storage(Vec::new());
-            // filter out the contracts that have been deleted in the meantime
-            contracts = contracts
-                .into_iter()
-                .filter(|contract_id| Contracts::<T>::contains_key(contract_id))
-                .collect::<Vec<_>>();
-            contracts.dedup();
 
             let contracts = ContractsToBillAt::<T>::get(current_index);
             if contracts.is_empty() {
@@ -973,23 +964,17 @@ impl<T: Config> Pallet<T> {
         return Err(<Error<T>>::OffchainSignedTxError);
     }
 
-    fn bill_contract_using_signed_transaction(
-        block_number: T::BlockNumber,
-        contract_id: u64,
-    ) -> Result<(), Error<T>> {
+    fn bill_contract_using_signed_transaction(contract_id: u64) -> Result<(), Error<T>> {
         let signer = Signer::<T, <T as pallet::Config>::AuthorityId>::any_account();
         if !signer.can_sign() {
             log::error!(
-                "failed billing contract {:?}: at block {:?} account cannot be used to sign transaction",
+                "failed billing contract {:?} account cannot be used to sign transaction",
                 contract_id,
-                block_number
             );
             return Err(<Error<T>>::OffchainSignedTxError);
         }
-        let result = signer.send_signed_transaction(|_acct| Call::bill_contract_for_block {
-            contract_id,
-            block_number,
-        });
+        let result =
+            signer.send_signed_transaction(|_acct| Call::bill_contract_for_block { contract_id });
 
         if let Some((acc, res)) = result {
             // if res is an error this means sending the transaction failed
@@ -998,9 +983,8 @@ impl<T: Config> Pallet<T> {
             // returns Err())
             if res.is_err() {
                 log::error!(
-                    "signed transaction failed for billing contract {:?} at block {:?} using account {:?}",
+                    "signed transaction failed for billing contract {:?} using account {:?}",
                     contract_id,
-                    block_number,
                     acc.id
                 );
                 return Err(<Error<T>>::OffchainSignedTxError);
@@ -1011,36 +995,6 @@ impl<T: Config> Pallet<T> {
         return Err(<Error<T>>::OffchainSignedTxError);
     }
 
-    fn append_failed_contract_ids_to_storage(failed_id: u64) {
-        log::info!(
-            "billing contract {:?} will be retried in the next block",
-            failed_id
-        );
-
-        let mut failed_ids = Self::get_failed_contract_ids_from_storage();
-        failed_ids.push(failed_id);
-
-        Self::save_failed_contract_ids_in_storage(failed_ids);
-    }
-
-    fn save_failed_contract_ids_in_storage(failed_ids: Vec<u64>) {
-        let s_contracts =
-            StorageValueRef::persistent(b"pallet-smart-contract::failed-contracts-when-billing");
-
-        s_contracts.set(&failed_ids);
-    }
-
-    fn get_failed_contract_ids_from_storage() -> Vec<u64> {
-        let s_contracts =
-            StorageValueRef::persistent(b"pallet-smart-contract::failed-contracts-when-billing");
-
-        if let Ok(Some(failed_contract_ids)) = s_contracts.get::<Vec<u64>>() {
-            return failed_contract_ids;
-        }
-
-        return Vec::new();
-    }
-
     // Bills a contract (NodeContract or NameContract)
     // Calculates how much TFT is due by the user and distributes the rewards
     fn bill_contract(contract_id: u64) -> DispatchResultWithPostInfo {
@@ -1096,13 +1050,15 @@ impl<T: Config> Pallet<T> {
             return Ok(().into());
         }
 
-        // Handle contract lock operations
-        // skip when the contract status was grace and changed to deleted because that means
-        // the grace period ended and there were still no funds
-        if !(was_grace && matches!(contract.state, types::ContractState::Deleted(_))) {
-            Self::handle_lock(contract, amount_due)?;
+        // If still in grace period, no need to continue doing locking and other stuff
+        if matches!(contract.state, types::ContractState::GracePeriod(_)) {
+            log::debug!("contract {} is still in grace", contract.contract_id);
+            return Ok(().into());
         }
 
+        // Handle contract lock operations
+        Self::handle_lock(contract, amount_due)?;
+
         // Always emit a contract billed event
         let contract_bill = types::ContractBill {
             contract_id: contract.contract_id,
@@ -1809,6 +1765,11 @@ impl<T: Config> Pallet<T> {
         let now = <frame_system::Pallet<T>>::block_number().saturated_into::<u64>();
         now % BillingFrequency::<T>::get()
     }
+
+    pub fn get_contract_index() -> u64 {
+        let now = <frame_system::Pallet<T>>::block_number().saturated_into::<u64>();
+        now % BillingFrequency::<T>::get()
+    }
 }
 
 impl<T: Config> ChangeNode<PubConfigOf<T>, InterfaceOf<T>> for Pallet<T> {

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

@@ -0,0 +1,91 @@
+use super::*;
+use frame_support::weights::Weight;
+
+pub mod v5 {
+    use super::*;
+    use crate::Config;
+
+    use frame_support::{pallet_prelude::Weight, traits::OnRuntimeUpgrade};
+    use sp_std::marker::PhantomData;
+    pub struct ContractMigrationV5<T: Config>(PhantomData<T>);
+
+    impl<T: Config> OnRuntimeUpgrade for ContractMigrationV5<T> {
+        #[cfg(feature = "try-runtime")]
+        fn pre_upgrade() -> Result<(), &'static str> {
+            info!("current pallet version: {:?}", PalletVersion::<T>::get());
+            assert!(PalletVersion::<T>::get() == types::StorageVersion::V5);
+
+            info!("👥  Smart Contract pallet to v6 passes PRE migrate checks ✅",);
+            Ok(())
+        }
+
+        fn on_runtime_upgrade() -> Weight {
+            migrate_to_version_6::<T>()
+        }
+
+        #[cfg(feature = "try-runtime")]
+        fn post_upgrade() -> Result<(), &'static str> {
+            info!("current pallet version: {:?}", PalletVersion::<T>::get());
+            assert!(PalletVersion::<T>::get() == types::StorageVersion::V6);
+
+            info!(
+                "👥  Smart Contract pallet to {:?} passes POST migrate checks ✅",
+                PalletVersion::<T>::get()
+            );
+
+            Ok(())
+        }
+    }
+}
+
+pub fn migrate_to_version_6<T: Config>() -> frame_support::weights::Weight {
+    if PalletVersion::<T>::get() == types::StorageVersion::V5 {
+        info!(
+            " >>> Starting contract pallet migration, pallet version: {:?}",
+            PalletVersion::<T>::get()
+        );
+
+        let mut migrated_count = 0;
+
+        // Collect ContractsToBillAt storage in memory
+        let contracts_to_bill_at = ContractsToBillAt::<T>::iter().collect::<Vec<_>>();
+
+        // Remove all items under ContractsToBillAt
+        frame_support::migration::remove_storage_prefix(
+            b"SmartContractModule",
+            b"ContractsToBillAt",
+            b"",
+        );
+
+        let billing_freq = 600;
+        BillingFrequency::<T>::put(billing_freq);
+
+        for (block_number, contract_ids) in contracts_to_bill_at {
+            migrated_count += 1;
+            // Construct new index
+            let index = (block_number - 1) % billing_freq;
+            // Reinsert items under the new key
+            info!(
+                "inserted contracts:{:?} at index: {:?}",
+                contract_ids.clone(),
+                index
+            );
+            ContractsToBillAt::<T>::insert(index, contract_ids);
+        }
+
+        info!(
+            " <<< Contracts storage updated! Migrated {} Contracts ✅",
+            migrated_count
+        );
+
+        // Update pallet storage version
+        PalletVersion::<T>::set(types::StorageVersion::V6);
+        info!(" <<< Storage version upgraded");
+
+        // Return the weight consumed by the migration.
+        T::DbWeight::get().reads_writes(migrated_count as Weight + 1, migrated_count as Weight + 1)
+    } else {
+        info!(" >>> Unused migration");
+        return 0;
+    }
+}

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

@@ -3,7 +3,7 @@ use std::{panic, thread};
 
 use super::*;
 use crate::name_contract::NameContractName;
-use crate::{self as pallet_smart_contract, types::BlockNumber};
+use crate::{self as pallet_smart_contract};
 use codec::{alloc::sync::Arc, Decode};
 use frame_support::{
     construct_runtime, parameter_types,
@@ -334,70 +334,57 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
     t
 }
 pub type TransactionCall = pallet_smart_contract::Call<TestRuntime>;
+pub type ExtrinsicResult = Result<PostDispatchInfo, DispatchErrorWithPostInfo>;
 
 #[derive(Default)]
 pub struct PoolState {
     /// A vector of calls that we expect should be executed
-    pub expected_calls: Vec<(TransactionCall, Result<(), ()>)>,
-    pub calls_to_execute: Vec<(TransactionCall, Result<(), ()>)>,
+    pub expected_calls: Vec<(TransactionCall, ExtrinsicResult, u64)>,
+    pub calls_to_execute: Vec<(TransactionCall, ExtrinsicResult, u64)>,
     pub i: usize,
 }
 
 impl PoolState {
     pub fn should_call_bill_contract(
         &mut self,
         contract_id: u64,
-        block_number: BlockNumber,
-        expected_result: Result<(), ()>,
+        expected_result: ExtrinsicResult,
+        block_number: u64,
     ) {
         self.expected_calls.push((
-            crate::Call::bill_contract_for_block {
-                contract_id,
-                block_number,
-            },
+            crate::Call::bill_contract_for_block { contract_id },
             expected_result,
+            block_number,
         ));
     }
 
-    pub fn should_call(&mut self, expected_call: TransactionCall, expected_result: Result<(), ()>) {
-        self.expected_calls.push((expected_call, expected_result));
-    }
-
-    pub fn execute_calls_and_check_results(&mut self) {
+    pub fn execute_calls_and_check_results(&mut self, block_number: u64) {
         if self.calls_to_execute.len() == 0 {
             return;
         }
-    
+
         // execute the calls that were submitted to the pool and compare the result
         for call_to_execute in self.calls_to_execute.iter() {
             let result = match call_to_execute.0 {
                 // matches bill_contract_for_block
-                crate::Call::bill_contract_for_block {
-                    contract_id,
-                    block_number,
-                } => SmartContractModule::bill_contract_for_block(
-                    Origin::signed(bob()),
-                    contract_id,
-                    block_number,
-                ),
+                crate::Call::bill_contract_for_block { contract_id } => {
+                    SmartContractModule::bill_contract_for_block(Origin::signed(bob()), contract_id)
+                }
                 // did not match anything => unkown call => this means you should add
                 // a capture for that function here
                 _ => panic!("Unknown call!"),
             };
-
-            let result = match result {
-                Ok(_) => Ok(()),
-                Err(_) => Err(()),
-            };
-
+
             // the call should return what we expect it to return
             assert_eq!(
                 call_to_execute.1, result,
                 "The result of call to {:?} was not as expected!",
                 call_to_execute.0
             );
+
+            assert_eq!(block_number, call_to_execute.2);
         }
-    
+
         self.calls_to_execute.clear();
     }
 }
@@ -412,10 +399,10 @@ impl Drop for PoolState {
 }
 
 /// Implementation of mocked transaction pool used for testing
-/// 
-/// This transaction pool mocks submitting the transactions to the pool. It does 
+///
+/// This transaction pool mocks submitting the transactions to the pool. It does
 /// not execute the transactions. Instead it keeps them in list. It does compare
-/// the submitted call to the expected call. 
+/// the submitted call to the expected call.
 #[derive(Default)]
 pub struct MockedTransactionPoolExt(Arc<RwLock<PoolState>>);
 
@@ -457,7 +444,10 @@ impl TransactionPool for MockedTransactionPoolExt {
             self.0.write().i = i + 1;
 
             // return the expected return value
-            return self.0.read().expected_calls[i].1;
+            return self.0.read().expected_calls[i]
+                .1
+                .map_err(|_| ())
+                .map(|_| ());
         }
 
         // we should not end here as it would mean we did not expect any more calls

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

@@ -1,8 +1,6 @@
 #![cfg(test)]
 
-use crate::mock::{
-    bob, Origin, PoolState, SmartContractModule, System, Timestamp,
-};
+use crate::mock::{PoolState, SmartContractModule, System, Timestamp};
 
 use codec::alloc::sync::Arc;
 use frame_support::traits::Hooks;