[refactor]: move instruction len from data_model into core

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
hyperledger-iroha · Jul 14, 2023 · fae6107 · fae6107
1 parent 1b22b5b
commit fae6107
Show file tree

Hide file tree

Showing 29 changed files with 204 additions and 240 deletions.
diff --git a/client/src/client.rs b/client/src/client.rs
@@ -21,7 +21,7 @@ use http_default::{AsyncWebSocketStream, WebSocketStream};
 use iroha_config::{client::Configuration, torii::uri, GetConfiguration, PostConfiguration};
 use iroha_crypto::{HashOf, KeyPair};
 use iroha_data_model::{
-    block::VersionedCommittedBlock, predicate::PredicateBox, prelude::*,
+    block::VersionedCommittedBlock, isi::Instruction, predicate::PredicateBox, prelude::*,
     transaction::TransactionPayload, ValidationFail,
 };
 use iroha_logger::prelude::*;

diff --git a/client/tests/integration/burn_public_keys.rs b/client/tests/integration/burn_public_keys.rs
@@ -2,7 +2,7 @@
 
 use iroha_client::client::{account, transaction, Client};
 use iroha_crypto::{KeyPair, PublicKey};
-use iroha_data_model::prelude::*;
+use iroha_data_model::{isi::Instruction, prelude::*};
 use test_network::*;
 
 fn submit_and_get(

diff --git a/client/tests/integration/events/pipeline.rs b/client/tests/integration/events/pipeline.rs
@@ -116,7 +116,14 @@ fn committed_block_must_be_available_in_kura() {
         .expect("Failed to submit transaction");
 
     let event = event_iter.next().expect("Block must be committed");
-    let Ok(Event::Pipeline(PipelineEvent { entity_kind: PipelineEntityKind::Block, status: PipelineStatus::Committed, hash })) = event else { panic!("Received unexpected event") };
+    let Ok(Event::Pipeline(PipelineEvent {
+        entity_kind: PipelineEntityKind::Block,
+        status: PipelineStatus::Committed,
+        hash,
+    })) = event
+    else {
+        panic!("Received unexpected event")
+    };
     let hash = HashOf::from_untyped_unchecked(hash);
 
     peer.iroha

diff --git a/client/tests/integration/query_errors.rs b/client/tests/integration/query_errors.rs
@@ -24,7 +24,7 @@ fn non_existent_account_is_specific_error() {
     match err {
         ClientQueryError::Validation(ValidationFail::QueryFailed(QueryExecutionFail::Find(
             err,
-        ))) => match *err {
+        ))) => match err {
             FindError::Domain(id) => assert_eq!(id.name.as_ref(), "regalia"),
             x => panic!("FindError::Domain expected, got {x:?}"),
         },

diff --git a/client/tests/integration/triggers/by_call_trigger.rs b/client/tests/integration/triggers/by_call_trigger.rs
@@ -200,8 +200,7 @@ fn trigger_should_not_be_executed_with_zero_repeats_count() -> Result<()> {
             .chain()
             .last()
             .expect("At least two error causes expected")
-            .downcast_ref::<Box<FindError>>()
-            .map(std::ops::Deref::deref),
+            .downcast_ref::<FindError>(),
         Some(FindError::Trigger(id)) if *id == trigger_id
     ));
 

diff --git a/core/src/queue.rs b/core/src/queue.rs
@@ -394,8 +394,9 @@ impl Queue {
     }
 
     fn decrease_per_user_tx_count(&self, account_id: &AccountId) {
-        let Entry::Occupied(mut occupied) = self.txs_per_user
-            .entry(account_id.clone()) else { panic!("Call to decrease always should be paired with increase count. This is a bug.") };
+        let Entry::Occupied(mut occupied) = self.txs_per_user.entry(account_id.clone()) else {
+            panic!("Call to decrease always should be paired with increase count. This is a bug.")
+        };
 
         let count = occupied.get_mut();
         if *count > 1 {

diff --git a/core/src/smartcontracts/isi/account.rs b/core/src/smartcontracts/isi/account.rs
@@ -50,7 +50,7 @@ pub mod isi {
             match wsv.asset(&asset_id) {
                 Err(err) => match err {
                     QueryExecutionFail::Find(find_err)
-                        if matches!(*find_err, FindError::Asset(_)) =>
+                        if matches!(find_err, FindError::Asset(_)) =>
                     {
                         assert_can_register(&asset_id.definition_id, wsv, &self.object.value)?;
                         let asset = wsv

diff --git a/core/src/smartcontracts/isi/asset.rs b/core/src/smartcontracts/isi/asset.rs
@@ -619,7 +619,7 @@ pub mod query {
                 .asset(&id)
                 .map_err(|asset_err| {
                     if let Err(definition_err) = wsv.asset_definition(&id.definition_id) {
-                        Error::Find(Box::new(definition_err))
+                        Error::Find(definition_err)
                     } else {
                         asset_err
                     }
@@ -657,7 +657,7 @@ pub mod query {
                 .map_err(|e| Error::Evaluate(e.to_string()))?;
             let asset = wsv.asset(&id).map_err(|asset_err| {
                 if let Err(definition_err) = wsv.asset_definition(&id.definition_id) {
-                    Error::Find(Box::new(definition_err))
+                    Error::Find(definition_err)
                 } else {
                     asset_err
                 }
@@ -670,7 +670,7 @@ pub mod query {
                 .map_err(|e| Error::Conversion(e.to_string()))?;
             Ok(store
                 .get(&key)
-                .ok_or_else(|| Error::Find(Box::new(FindError::MetadataKey(key))))?
+                .ok_or_else(|| Error::Find(FindError::MetadataKey(key)))?
                 .clone())
         }
     }

diff --git a/core/src/smartcontracts/isi/block.rs b/core/src/smartcontracts/isi/block.rs
@@ -47,7 +47,7 @@ impl ValidQuery for FindBlockHeaderByHash {
         let block = wsv
             .all_blocks()
             .find(|block| block.hash() == hash)
-            .ok_or_else(|| QueryExecutionFail::Find(Box::new(FindError::Block(hash))))?;
+            .ok_or_else(|| QueryExecutionFail::Find(FindError::Block(hash)))?;
 
         Ok(block.as_v1().header.clone())
     }

diff --git a/core/src/smartcontracts/isi/mod.rs b/core/src/smartcontracts/isi/mod.rs
@@ -443,7 +443,7 @@ pub mod prelude {
 mod tests {
     #![allow(clippy::restriction)]
 
-    use core::str::FromStr;
+    use core::str::FromStr as _;
     use std::sync::Arc;
 
     use iroha_crypto::KeyPair;

diff --git a/core/src/smartcontracts/isi/query.rs b/core/src/smartcontracts/isi/query.rs
@@ -100,7 +100,9 @@ mod tests {
     use std::str::FromStr as _;
 
     use iroha_crypto::{Hash, HashOf, KeyPair};
-    use iroha_data_model::{block::VersionedCommittedBlock, transaction::TransactionLimits};
+    use iroha_data_model::{
+        block::VersionedCommittedBlock, query::error::FindError, transaction::TransactionLimits,
+    };
     use once_cell::sync::Lazy;
 
     use super::*;
@@ -384,7 +386,10 @@ mod tests {
             .sign(ALICE_KEYS.clone())?;
         let wrong_hash = unapplied_tx.hash();
         let not_found = FindTransactionByHash::new(wrong_hash).execute(&wsv);
-        assert!(not_found.is_err());
+        assert!(matches!(
+            not_found,
+            Err(Error::Find(FindError::Transaction(_)))
+        ));
 
         let found_accepted = FindTransactionByHash::new(va_tx.hash()).execute(&wsv)?;
         if found_accepted.transaction.error.is_none() {

diff --git a/core/src/smartcontracts/isi/triggers/mod.rs b/core/src/smartcontracts/isi/triggers/mod.rs
@@ -225,7 +225,7 @@ pub mod query {
             } = wsv
                 .triggers()
                 .inspect_by_id(&id, |action| action.clone_and_box())
-                .ok_or_else(|| Error::Find(Box::new(FindError::Trigger(id.clone()))))?;
+                .ok_or_else(|| Error::Find(FindError::Trigger(id.clone())))?;
 
             let action =
                 Action::new(loaded_executable, repeats, authority, filter).with_metadata(metadata);
@@ -253,7 +253,7 @@ pub mod query {
                         .map(Clone::clone)
                         .ok_or_else(|| FindError::MetadataKey(key.clone()).into())
                 })
-                .ok_or_else(|| Error::Find(Box::new(FindError::Trigger(id))))?
+                .ok_or_else(|| Error::Find(FindError::Trigger(id)))?
         }
     }
 

diff --git a/core/src/smartcontracts/isi/world.rs b/core/src/smartcontracts/isi/world.rs
@@ -448,7 +448,7 @@ pub mod query {
             iroha_logger::trace!(%role_id);
 
             wsv.world.roles.get(&role_id).map_or_else(
-                || Err(Error::Find(Box::new(FindError::Role(role_id)))),
+                || Err(Error::Find(FindError::Role(role_id))),
                 |role_ref| Ok(role_ref.clone()),
             )
         }

diff --git a/core/src/tx.rs b/core/src/tx.rs
@@ -46,6 +46,34 @@ pub enum AcceptTransactionFail {
     SignatureVerification(#[source] SignatureVerificationFail<TransactionPayload>),
 }
 
+fn instruction_size(isi: &InstructionBox) -> usize {
+    use InstructionBox::*;
+
+    match isi {
+        Register(isi) => isi.object.len() + 1,
+        Unregister(isi) => isi.object_id.len() + 1,
+        Mint(isi) => isi.destination_id.len() + isi.object.len() + 1,
+        Burn(isi) => isi.destination_id.len() + isi.object.len() + 1,
+        Transfer(isi) => isi.destination_id.len() + isi.object.len() + isi.source_id.len() + 1,
+        If(isi) => {
+            let otherwise = isi.otherwise.as_ref().map_or(0, instruction_size);
+            isi.condition.len() + instruction_size(&isi.then) + otherwise + 1
+        }
+        Pair(isi) => {
+            instruction_size(&isi.left_instruction) + instruction_size(&isi.right_instruction) + 1
+        }
+        Sequence(isi) => isi.instructions.iter().map(instruction_size).sum::<usize>() + 1,
+        SetKeyValue(isi) => isi.object_id.len() + isi.key.len() + isi.value.len() + 1,
+        RemoveKeyValue(isi) => isi.object_id.len() + isi.key.len() + 1,
+        Grant(isi) => isi.object.len() + isi.destination_id.len() + 1,
+        Revoke(isi) => isi.object.len() + isi.destination_id.len() + 1,
+        SetParameter(isi) => isi.parameter.len() + 1,
+        NewParameter(isi) => isi.parameter.len() + 1,
+        Upgrade(isi) => isi.object.len() + 1,
+        Fail(_) | ExecuteTrigger(_) => 1,
+    }
+}
+
 impl AcceptedTransaction {
     /// Accept genesis transaction. Transition from [`GenesisTransaction`] to [`AcceptedTransaction`].
     pub fn accept_genesis(tx: GenesisTransaction) -> Self {
@@ -65,7 +93,7 @@ impl AcceptedTransaction {
             Executable::Instructions(instructions) => {
                 let instruction_count: u64 = instructions
                     .iter()
-                    .map(InstructionBox::len)
+                    .map(instruction_size)
                     .sum::<usize>()
                     .try_into()
                     .expect("`usize` should always fit in `u64`");
@@ -295,3 +323,67 @@ impl TransactionValidator {
             })
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use core::str::FromStr as _;
+
+    use super::*;
+
+    fn if_instruction(
+        c: impl Into<Expression>,
+        then: InstructionBox,
+        otherwise: Option<InstructionBox>,
+    ) -> InstructionBox {
+        let condition: Expression = c.into();
+        let condition = EvaluatesTo::new_unchecked(condition);
+        Conditional {
+            condition,
+            then,
+            otherwise,
+        }
+        .into()
+    }
+
+    fn fail() -> InstructionBox {
+        FailBox {
+            message: String::default(),
+        }
+        .into()
+    }
+
+    #[test]
+    fn len_empty_sequence() {
+        assert_eq!(instruction_size(&SequenceBox::new(vec![]).into()), 1);
+    }
+
+    #[test]
+    fn len_if_one_branch() {
+        let instructions = vec![if_instruction(
+            ContextValue {
+                value_name: Name::from_str("a").expect("Cannot fail."),
+            },
+            fail(),
+            None,
+        )];
+
+        assert_eq!(instruction_size(&SequenceBox::new(instructions).into()), 4);
+    }
+
+    #[test]
+    fn len_sequence_if() {
+        let instructions = vec![
+            fail(),
+            if_instruction(
+                ContextValue {
+                    value_name: Name::from_str("b").expect("Cannot fail."),
+                },
+                fail(),
+                Some(fail()),
+            ),
+            fail(),
+        ];
+
+        assert_eq!(instruction_size(&SequenceBox::new(instructions).into()), 7);
+    }
+}
diff --git a/core/src/wsv.rs b/core/src/wsv.rs
@@ -474,7 +474,7 @@ impl WorldStateView {
                 account
                     .assets
                     .get(id)
-                    .ok_or_else(|| QueryExecutionFail::from(Box::new(FindError::Asset(id.clone()))))
+                    .ok_or_else(|| QueryExecutionFail::from(FindError::Asset(id.clone())))
                     .map(Clone::clone)
             },
         )?

diff --git a/core/test_network/src/lib.rs b/core/test_network/src/lib.rs
@@ -21,7 +21,7 @@ use iroha_config::{
     torii::Configuration as ToriiConfiguration,
 };
 use iroha_core::prelude::*;
-use iroha_data_model::{peer::Peer as DataModelPeer, prelude::*};
+use iroha_data_model::{isi::Instruction, peer::Peer as DataModelPeer, prelude::*};
 use iroha_genesis::{GenesisNetwork, RawGenesisBlock};
 use iroha_logger::{Configuration as LoggerConfiguration, InstrumentFutures};
 use iroha_primitives::addr::{socket_addr, SocketAddr};

diff --git a/data_model/derive/src/model.rs b/data_model/derive/src/model.rs
@@ -15,7 +15,10 @@ pub fn impl_model(input: &syn::ItemMod) -> TokenStream {
     } = input;
 
     let syn::Visibility::Public(vis_public) = vis else {
-        abort!(input, "The `model` attribute can only be used on public modules");
+        abort!(
+            input,
+            "The `model` attribute can only be used on public modules"
+        );
     };
     if ident != "model" {
         abort!(

diff --git a/data_model/derive/tests/ui_fail/transparent_api_private_item.stderr b/data_model/derive/tests/ui_fail/transparent_api_private_item.stderr
@@ -8,4 +8,4 @@ note: the struct `QueryPayload` is defined here
  --> $WORKSPACE/data_model/src/query.rs
   |
   |     pub use self::model::*;
-  |             ^^^^^^^^^^^^^^
+  |             ^^^^^^^^^^^