addressing feedback - backward incompatible apis + test simplification

domains/client/block-preprocessor/src/lib.rs

@@ -20,7 +20,7 @@ use codec::{Decode, Encode};
 use domain_runtime_primitives::opaque::AccountId;
 use domain_runtime_primitives::DomainCoreApi;
 use sc_client_api::BlockBackend;
-use sp_api::{ApiExt, HashT, ProvideRuntimeApi};
+use sp_api::{HashT, ProvideRuntimeApi};
 use sp_blockchain::HeaderBackend;
 use sp_core::H256;
 use sp_domains::extrinsics::deduplicate_and_shuffle_extrinsics;
@@ -268,14 +268,6 @@ where
         //
         // NOTE: for each extrinsic the checking order must follow `InvalidBundleType::checking_order`
         let runtime_api = self.client.runtime_api();
-        let api_version = runtime_api
-            .api_version::<dyn DomainCoreApi<Block>>(at)
-            .map_err(sp_blockchain::Error::RuntimeApiError)?
-            .ok_or_else(|| {
-                sp_blockchain::Error::Application(
-                    format!("Could not find `DomainCoreApi` api for block `{:?}`.", at).into(),
-                )
-            })?;
         for (index, opaque_extrinsic) in bundle.extrinsics.iter().enumerate() {
             let Ok(extrinsic) =
                 <<Block as BlockT>::Extrinsic>::decode(&mut opaque_extrinsic.encode().as_slice())
@@ -313,23 +305,9 @@ where
             // between them.
             // Using `check_transaction_and_do_pre_dispatch` instead of `check_transaction_validity`
             // to maintain side-effect in the storage buffer.
-            let is_legal_tx = if api_version == 2 {
-                runtime_api.check_transaction_and_do_pre_dispatch(
-                    at,
-                    &extrinsic,
-                    domain_block_number,
-                    at,
-                )?
-            } else {
-                #[allow(deprecated)] // old check_transaction_validity
-                runtime_api.check_transaction_validity_before_version_2(
-                    at,
-                    &extrinsic,
-                    domain_block_number,
-                    at,
-                )?
-            }
-            .is_ok();
+            let is_legal_tx = runtime_api
+                .check_transaction_and_do_pre_dispatch(at, &extrinsic, domain_block_number, at)?
+                .is_ok();
 
             if !is_legal_tx {
                 // TODO: Generate a fraud proof for this invalid bundle

domains/client/block-preprocessor/src/stateless_runtime.rs

@@ -1,12 +1,13 @@
 use codec::{Codec, Encode};
 use domain_runtime_primitives::opaque::AccountId;
-use domain_runtime_primitives::{CheckTxValidityError, DomainCoreApi};
+use domain_runtime_primitives::DomainCoreApi;
 use sc_executor::RuntimeVersionOf;
 use sp_api::{ApiError, BlockT, Core, Hasher, RuntimeVersion};
 use sp_core::traits::{CallContext, CodeExecutor, FetchRuntimeCode, RuntimeCode};
 use sp_messenger::messages::ExtractedStateRootsFromProof;
 use sp_messenger::MessengerApi;
 use sp_runtime::traits::NumberFor;
+use sp_runtime::transaction_validity::TransactionValidityError;
 use sp_runtime::Storage;
 use sp_state_machine::BasicExternalities;
 use std::borrow::Cow;
@@ -212,29 +213,13 @@ where
         )
     }
 
-    /// This is stateful runtime api call and require setting of storage keys.
-    pub fn check_transaction_validity(
-        &self,
-        uxt: &<Block as BlockT>::Extrinsic,
-        block_number: NumberFor<Block>,
-        block_hash: <Block as BlockT>::Hash,
-    ) -> Result<Result<(), CheckTxValidityError>, ApiError> {
-        <Self as DomainCoreApi<Block>>::check_transaction_validity(
-            self,
-            Default::default(),
-            uxt,
-            block_number,
-            block_hash,
-        )
-    }
-
     /// This is stateful runtime api call and require setting of storage keys.
     pub fn check_transaction_and_do_pre_dispatch(
         &self,
         uxt: &<Block as BlockT>::Extrinsic,
         block_number: NumberFor<Block>,
         block_hash: <Block as BlockT>::Hash,
-    ) -> Result<Result<(), CheckTxValidityError>, ApiError> {
+    ) -> Result<Result<(), TransactionValidityError>, ApiError> {
         <Self as DomainCoreApi<Block>>::check_transaction_and_do_pre_dispatch(
             self,
             Default::default(),

domains/client/domain-operator/src/domain_bundle_proposer.rs

@@ -115,18 +115,6 @@ where
             // We are using one runtime api instance here to maintain storage changes in the instance's internal buffer
             // between runtime calls done in this loop.
             let runtime_api_instance = self.client.runtime_api();
-            let api_version = runtime_api_instance
-                .api_version::<dyn DomainCoreApi<Block>>(parent_hash)
-                .map_err(sp_blockchain::Error::RuntimeApiError)?
-                .ok_or_else(|| {
-                    sp_blockchain::Error::Application(
-                        format!(
-                            "Could not find `DomainCoreApi` api for block `{:?}`.",
-                            parent_hash
-                        )
-                        .into(),
-                    )
-                })?;
             for pending_tx in pending_iterator {
                 let pending_tx_data = pending_tx.data();
 
@@ -176,22 +164,13 @@ where
                 // some other checks, its side effect will still be part of RuntimeApiImpl's changes buffer.
                 let transaction_validity_result =
                     runtime_api_instance.execute_in_transaction(|api| {
-                        let transaction_validity_result = if api_version == 2 {
-                            api.check_transaction_and_do_pre_dispatch(
+                        let transaction_validity_result = api
+                            .check_transaction_and_do_pre_dispatch(
                                 parent_hash,
                                 pending_tx_data,
                                 parent_number,
                                 parent_hash,
-                            )
-                        } else {
-                            #[allow(deprecated)] // old check_transaction_validity
-                            api.check_transaction_validity_before_version_2(
-                                parent_hash,
-                                pending_tx_data,
-                                parent_number,
-                                parent_hash,
-                            )
-                        };
+                            );
                         // Only commit, if there are no errors (both ApiError and CheckTxValidityError)
                         if let Ok(Ok(_)) = transaction_validity_result {
                             sp_api::TransactionOutcome::Commit(transaction_validity_result)

domains/client/domain-operator/src/tests.rs

@@ -2769,7 +2769,7 @@ async fn test_domain_transaction_fee_and_operator_reward() {
     // Transaction fee (including the tip) is deducted from alice's account
     let alice_free_balance_changes =
         pre_alice_free_balance - alice.free_balance(Alice.to_account_id());
-    assert!(alice_free_balance_changes >= tip as u128);
+    assert!(alice_free_balance_changes >= tip);
 
     // All the transaction fee is collected as operator reward
     assert_eq!(alice_free_balance_changes, receipt.total_rewards);

domains/primitives/runtime/src/lib.rs

@@ -20,9 +20,7 @@
 use parity_scale_codec::{Decode, Encode};
 use scale_info::TypeInfo;
 use sp_runtime::generic::{Era, UncheckedExtrinsic};
-use sp_runtime::traits::{
-    Block as BlockT, Convert, IdentifyAccount, LookupError, NumberFor, Verify,
-};
+use sp_runtime::traits::{Block as BlockT, Convert, IdentifyAccount, NumberFor, Verify};
 use sp_runtime::transaction_validity::TransactionValidityError;
 use sp_runtime::{Digest, MultiAddress, MultiSignature};
 use sp_std::vec::Vec;
@@ -136,36 +134,8 @@ pub mod opaque {
     pub type AccountId = Vec<u8>;
 }
 
-#[derive(Debug, PartialEq, Eq, Encode, Decode, TypeInfo)]
-pub enum CheckTxValidityError {
-    /// Can not find the sender from address.
-    Lookup,
-    /// Unable to extract signer from tx
-    UnableToExtractSigner { error: TransactionValidityError },
-    /// Transaction is invalid.
-    InvalidTransaction {
-        /// Concrete transaction validity error type.
-        error: TransactionValidityError,
-        /// Storage keys of state accessed in the validation.
-        storage_keys: Vec<Vec<u8>>,
-    },
-}
-
-impl From<LookupError> for CheckTxValidityError {
-    fn from(_lookup_error: LookupError) -> Self {
-        Self::Lookup
-    }
-}
-
-#[derive(Debug, PartialEq, Eq, Encode, Decode, TypeInfo)]
-pub enum VerifyTxValidityError {
-    /// Failed to decode the opaque account id into the runtime account type.
-    FailedToDecodeAccountId,
-}
-
 sp_api::decl_runtime_apis! {
     /// Base API that every domain runtime must implement.
-    #[api_version(2)]
     pub trait DomainCoreApi {
         /// Extracts the optional signer per extrinsic.
         fn extract_signer(
@@ -196,46 +166,12 @@ sp_api::decl_runtime_apis! {
         /// Returns true if the extrinsic is an inherent extrinsic.
         fn is_inherent_extrinsic(extrinsic: &<Block as BlockT>::Extrinsic) -> bool;
 
-        /// Checks the validity of extrinsic.
-        #[changed_in(2)]
-        fn check_transaction_validity(
-            uxt: &<Block as BlockT>::Extrinsic,
-            block_number: NumberFor<Block>,
-            block_hash: <Block as BlockT>::Hash
-        ) -> Result<(), CheckTxValidityError>;
-
-        /// Checks the validity of extrinsic
-        /// v2 can check validity of unsigned and self contained extrinsics as well.
-        fn check_transaction_validity(
-            uxt: &<Block as BlockT>::Extrinsic,
-            block_number: NumberFor<Block>,
-            block_hash: <Block as BlockT>::Hash
-        ) -> Result<(), CheckTxValidityError>;
-
-        /// New in v2.
-        /// Checks transaction and also perform pre dispatch on transaction.
-        /// This is useful to maintain side effect in the RuntimeApi instance's storage buffer.
+        /// Checks the validity of extrinsic + do pre_dispatch as well.
         fn check_transaction_and_do_pre_dispatch(
             uxt: &<Block as BlockT>::Extrinsic,
             block_number: NumberFor<Block>,
             block_hash: <Block as BlockT>::Hash
-        ) -> Result<(), CheckTxValidityError>;
-
-        /// Returns the storage keys of states accessed in the API `check_transaction_validity`.
-        #[changed_in(2)]
-        fn storage_keys_for_verifying_transaction_validity(
-            account_id: opaque::AccountId,
-            block_number: NumberFor<Block>,
-            tx_era: Option<Era>,
-        ) -> Result<Vec<Vec<u8>>, VerifyTxValidityError>;
-
-        /// Returns the storage keys of states accessed in the API `check_transaction_validity`
-        /// In v2, signer account id is optional to support getting storage keys for unsigned tx as well.
-        fn storage_keys_for_verifying_transaction_validity(
-            maybe_account_id: Option<opaque::AccountId>,
-            block_number: NumberFor<Block>,
-            tx_era: Option<Era>,
-        ) -> Result<Vec<Vec<u8>>, VerifyTxValidityError>;
+        ) -> Result<(), TransactionValidityError>;
 
         /// Returns extrinsic Era if present
         fn extrinsic_era(