refactor: introduce FindTriggers query, remove FindTriggerById (#…

…5040)

Signed-off-by: ⭐️NINIKA⭐️ <[email protected]>

client/src/client.rs

@@ -1041,11 +1041,6 @@ pub mod trigger {
     pub fn all_ids() -> FindActiveTriggerIds {
         FindActiveTriggerIds
     }
-
-    /// Construct a query to get a trigger by its id
-    pub fn by_id(trigger_id: TriggerId) -> FindTriggerById {
-        FindTriggerById::new(trigger_id)
-    }
 }
 
 pub mod permission {

client/tests/integration/multisig.rs

@@ -10,7 +10,10 @@ use iroha::{
         transaction::{TransactionBuilder, WasmSmartContract},
     },
 };
-use iroha_data_model::{parameter::SmartContractParameter, query::builder::SingleQueryError};
+use iroha_data_model::{
+    parameter::SmartContractParameter,
+    query::{builder::SingleQueryError, trigger::FindTriggers},
+};
 use nonzero_ext::nonzero;
 use test_network::*;
 use test_samples::{gen_account_in, ALICE_ID};
@@ -94,7 +97,9 @@ fn mutlisig() -> Result<()> {
 
     // Check that multisig trigger exist
     let trigger = test_client
-        .query_single(client::trigger::by_id(multisig_trigger_id.clone()))
+        .query(FindTriggers::new())
+        .filter_with(|trigger| trigger.id.eq(multisig_trigger_id.clone()))
+        .execute_single()
         .expect("multisig trigger should be created after the call to register multisig trigger");
 
     assert_eq!(trigger.id(), &multisig_trigger_id);

client/tests/integration/triggers/by_call_trigger.rs

@@ -7,10 +7,11 @@ use iroha::{
     crypto::KeyPair,
     data_model::{
         prelude::*,
-        query::error::{FindError, QueryExecutionFail},
+        query::error::FindError,
         transaction::{Executable, WasmSmartContract},
     },
 };
+use iroha_data_model::query::{builder::SingleQueryError, trigger::FindTriggers};
 use iroha_executor_data_model::permission::trigger::CanRegisterUserTrigger;
 use iroha_genesis::GenesisBlock;
 use iroha_logger::info;
@@ -203,16 +204,22 @@ fn trigger_should_not_be_executed_with_zero_repeats_count() -> Result<()> {
     //     .expect_err("Error expected");
     // iroha_logger::info!(?error);
 
-    assert!(matches!(
-        test_client
-            .submit_blocking(execute_trigger)
-            .expect_err("Error expected")
-            .chain()
-            .last()
-            .expect("At least two error causes expected")
-            .downcast_ref::<QueryExecutionFail>(),
-        Some(QueryExecutionFail::Find(FindError::Trigger(id))) if *id == trigger_id
-    ));
+    let error = test_client
+        .submit_blocking(execute_trigger)
+        .expect_err("Error expected");
+    let downcasted_error = error
+        .chain()
+        .last()
+        .expect("At least two error causes expected")
+        .downcast_ref::<FindError>();
+    assert!(
+        matches!(
+            downcasted_error,
+            Some(FindError::Trigger(id)) if *id == trigger_id
+        ),
+        "Unexpected error received: {:?}",
+        error
+    );
 
     // Checking results
     let new_asset_value = get_asset_value(&mut test_client, asset_id);
@@ -334,10 +341,10 @@ fn only_account_with_permission_can_register_trigger() -> Result<()> {
         .submit_blocking(Register::trigger(trigger))
         .expect("Trigger should be registered!");
 
-    let find_trigger = FindTriggerById {
-        id: trigger_id.clone(),
-    };
-    let found_trigger = test_client.query_single(find_trigger)?;
+    let found_trigger = test_client
+        .query(FindTriggers::new())
+        .filter_with(|trigger| trigger.id.eq(trigger_id.clone()))
+        .execute_single()?;
 
     assert_eq!(found_trigger.id, trigger_id);
 
@@ -368,10 +375,10 @@ fn unregister_trigger() -> Result<()> {
     test_client.submit_blocking(register_trigger)?;
 
     // Finding trigger
-    let find_trigger = FindTriggerById {
-        id: trigger_id.clone(),
-    };
-    let found_trigger = test_client.query_single(find_trigger.clone())?;
+    let found_trigger = test_client
+        .query(FindTriggers::new())
+        .filter_with(|trigger| trigger.id.eq(trigger_id.clone()))
+        .execute_single()?;
     let found_action = found_trigger.action;
     let Executable::Instructions(found_instructions) = found_action.executable else {
         panic!("Expected instructions");
@@ -388,11 +395,17 @@ fn unregister_trigger() -> Result<()> {
     assert_eq!(found_trigger, trigger);
 
     // Unregistering trigger
-    let unregister_trigger = Unregister::trigger(trigger_id);
+    let unregister_trigger = Unregister::trigger(trigger_id.clone());
     test_client.submit_blocking(unregister_trigger)?;
 
     // Checking result
-    assert!(test_client.query_single(find_trigger).is_err());
+    assert!(matches!(
+        test_client
+            .query(FindTriggers::new())
+            .filter_with(|trigger| trigger.id.eq(trigger_id.clone()))
+            .execute_single(),
+        Err(SingleQueryError::ExpectedOneGotNone)
+    ));
 
     Ok(())
 }
@@ -610,10 +623,9 @@ fn unregistering_one_of_two_triggers_with_identical_wasm_should_not_cause_origin
 
     test_client.submit_blocking(Unregister::trigger(first_trigger_id))?;
     let got_second_trigger = test_client
-        .query_single(FindTriggerById {
-            id: second_trigger_id,
-        })
-        .expect("Failed to request second trigger");
+        .query(FindTriggers::new())
+        .filter_with(|trigger| trigger.id.eq(second_trigger_id.clone()))
+        .execute_single()?;
 
     assert_eq!(got_second_trigger, second_trigger);
 

client/tests/integration/triggers/orphans.rs

@@ -1,14 +1,14 @@
-use iroha::{
-    client::{trigger, Client},
-    data_model::prelude::*,
-};
+use iroha::{client::Client, data_model::prelude::*};
+use iroha_data_model::query::trigger::FindTriggers;
 use test_network::{wait_for_genesis_committed, Peer, PeerBuilder};
 use test_samples::gen_account_in;
 use tokio::runtime::Runtime;
 
 fn find_trigger(iroha: &Client, trigger_id: TriggerId) -> Option<TriggerId> {
     iroha
-        .query_single(trigger::by_id(trigger_id))
+        .query(FindTriggers::new())
+        .filter_with(|trigger| trigger.id.eq(trigger_id.clone()))
+        .execute_single()
         .ok()
         .map(|trigger| trigger.id)
 }

core/src/smartcontracts/isi/query.rs

@@ -245,9 +245,6 @@ impl ValidQueryRequest {
                     SingularQueryBox::FindParameters(q) => {
                         SingularQueryOutputBox::from(q.execute(state)?)
                     }
-                    SingularQueryBox::FindTriggerById(q) => {
-                        SingularQueryOutputBox::from(q.execute(state)?)
-                    }
                     SingularQueryBox::FindDomainMetadata(q) => {
                         SingularQueryOutputBox::from(q.execute(state)?)
                     }
@@ -314,6 +311,10 @@ impl ValidQueryRequest {
                         ValidQuery::execute(q.query, q.predicate, state)?,
                         &iter_query.params,
                     )?,
+                    QueryBox::FindTriggers(q) => apply_query_postprocessing(
+                        ValidQuery::execute(q.query, q.predicate, state)?,
+                        &iter_query.params,
+                    )?,
                     QueryBox::FindTransactions(q) => apply_query_postprocessing(
                         ValidQuery::execute(q.query, q.predicate, state)?,
                         &iter_query.params,

core/src/smartcontracts/isi/triggers/mod.rs

@@ -325,6 +325,7 @@ pub mod query {
         query::{
             error::QueryExecutionFail as Error,
             predicate::{predicate_atoms::trigger::TriggerIdPredicateBox, CompoundPredicate},
+            trigger::FindTriggers,
         },
         trigger::{Trigger, TriggerId},
     };
@@ -353,27 +354,27 @@ pub mod query {
         }
     }
 
-    impl ValidSingularQuery for FindTriggerById {
-        #[metrics(+"find_trigger_by_id")]
-        fn execute(&self, state_ro: &impl StateReadOnly) -> Result<Trigger, Error> {
-            let id = &self.id;
-            iroha_logger::trace!(%id);
-            // Can't use just `LoadedActionTrait::clone_and_box` cause this will trigger lifetime mismatch
-            #[allow(clippy::redundant_closure_for_method_calls)]
-            let loaded_action = state_ro
-                .world()
-                .triggers()
-                .inspect_by_id(id, |action| action.clone_and_box())
-                .ok_or_else(|| Error::Find(FindError::Trigger(id.clone())))?;
+    impl ValidQuery for FindTriggers {
+        #[metrics(+"find_triggers")]
+        fn execute(
+            self,
+            filter: CompoundPredicate<TriggerPredicateBox>,
+            state_ro: &impl StateReadOnly,
+        ) -> Result<impl Iterator<Item = Self::Item>, Error> {
+            let triggers = state_ro.world().triggers();
 
-            let action = state_ro
-                .world()
-                .triggers()
-                .get_original_action(loaded_action)
-                .into();
+            Ok(triggers
+                   .ids_iter()
+                   .map(|id| {
+                       let action = triggers.inspect_by_id(id, |action| action.clone_and_box())
+                           .expect("INTERNAL BUG: Trigger Id is in the list of ids but not in the triggers map");
+
+                       let action = triggers.get_original_action(action)
+                           .into();
 
-            // TODO: Should we redact the metadata if the account is not the authority/owner?
-            Ok(Trigger::new(id.clone(), action))
+                       Trigger::new(id.clone(), action)
+                   })
+                   .filter(move |trigger| filter.applies(trigger)))
         }
     }
 

data_model/src/lib.rs

@@ -140,7 +140,7 @@ mod seal {
         FindPermissionsByAccountId,
         FindExecutorDataModel,
         FindActiveTriggerIds,
-        FindTriggerById,
+        FindTriggers,
         FindTriggerMetadata,
         FindRoles,
         FindRoleIds,

data_model/src/query/mod.rs

@@ -32,7 +32,7 @@ use crate::{
     role::{Role, RoleId},
     seal::Sealed,
     transaction::{CommittedTransaction, SignedTransaction},
-    trigger::TriggerId,
+    trigger::{Trigger, TriggerId},
 };
 
 pub mod builder;
@@ -99,6 +99,7 @@ mod model {
 
         FindPeers(QueryWithFilterFor<FindPeers>),
         FindActiveTriggerIds(QueryWithFilterFor<FindActiveTriggerIds>),
+        FindTriggers(QueryWithFilterFor<FindTriggers>),
         FindTransactions(QueryWithFilterFor<FindTransactions>),
         FindBlocks(QueryWithFilterFor<FindBlocks>),
         FindBlockHeaders(QueryWithFilterFor<FindBlockHeaders>),
@@ -122,6 +123,7 @@ mod model {
         Peer(Vec<Peer>),
         RoleId(Vec<RoleId>),
         TriggerId(Vec<TriggerId>),
+        Trigger(Vec<Trigger>),
         Block(Vec<SignedBlock>),
         BlockHeader(Vec<BlockHeader>),
     }
@@ -134,7 +136,6 @@ mod model {
         FindAssetQuantityById(FindAssetQuantityById),
         FindExecutorDataModel(FindExecutorDataModel),
         FindParameters(FindParameters),
-        FindTriggerById(FindTriggerById),
 
         FindDomainMetadata(FindDomainMetadata),
         FindAccountMetadata(FindAccountMetadata),
@@ -273,6 +274,7 @@ impl QueryOutputBatchBox {
             (Self::Peer(v1), Self::Peer(v2)) => v1.extend(v2),
             (Self::RoleId(v1), Self::RoleId(v2)) => v1.extend(v2),
             (Self::TriggerId(v1), Self::TriggerId(v2)) => v1.extend(v2),
+            (Self::Trigger(v1), Self::Trigger(v2)) => v1.extend(v2),
             (Self::Block(v1), Self::Block(v2)) => v1.extend(v2),
             (Self::BlockHeader(v1), Self::BlockHeader(v2)) => v1.extend(v2),
             _ => panic!("Cannot extend different types of IterableQueryOutputBatchBox"),
@@ -294,6 +296,7 @@ impl QueryOutputBatchBox {
             Self::Peer(v) => v.len(),
             Self::RoleId(v) => v.len(),
             Self::TriggerId(v) => v.len(),
+            Self::Trigger(v) => v.len(),
             Self::Block(v) => v.len(),
             Self::BlockHeader(v) => v.len(),
         }
@@ -559,6 +562,7 @@ impl_iter_queries! {
     FindDomains => crate::domain::Domain,
     FindPeers => crate::peer::Peer,
     FindActiveTriggerIds => crate::trigger::TriggerId,
+    FindTriggers => crate::trigger::Trigger,
     FindTransactions => TransactionQueryOutput,
     FindAccountsWithAsset => crate::account::Account,
     FindBlockHeaders => crate::block::BlockHeader,
@@ -572,7 +576,6 @@ impl_singular_queries! {
     FindAssetDefinitionMetadata => JsonString,
     FindDomainMetadata => JsonString,
     FindParameters => crate::parameter::Parameters,
-    FindTriggerById => crate::trigger::Trigger,
     FindTriggerMetadata => JsonString,
     FindExecutorDataModel => crate::executor::ExecutorDataModel,
 }
@@ -901,16 +904,11 @@ pub mod trigger {
         #[ffi_type]
         pub struct FindActiveTriggerIds;
 
-        /// Find Trigger given its ID.
-        #[derive(Display)]
-        #[display(fmt = "Find `{id}` trigger")]
-        #[repr(transparent)]
-        // SAFETY: `FindTriggerById` has no trap representation in `TriggerId`
-        #[ffi_type(unsafe {robust})]
-        pub struct FindTriggerById {
-            /// The Identification of the trigger to be found.
-            pub id: TriggerId,
-        }
+        /// Find all currently active (as in not disabled and/or expired) triggers.
+        #[derive(Copy, Display)]
+        #[display(fmt = "Find all triggers")]
+        #[ffi_type]
+        pub struct FindTriggers;
 
         /// Find Trigger's metadata key-value pairs.
         #[derive(Display)]
@@ -926,7 +924,7 @@ pub mod trigger {
 
     pub mod prelude {
         //! Prelude Re-exports most commonly used traits, structs and macros from this crate.
-        pub use super::{FindActiveTriggerIds, FindTriggerById, FindTriggerMetadata};
+        pub use super::{FindActiveTriggerIds, FindTriggerMetadata, FindTriggers};
     }
 }