review: replace host.submit with executor.visit_*

Signed-off-by: Shunkichi Sato <[email protected]>

crates/iroha/tests/multisig.rs

@@ -12,6 +12,7 @@ use iroha::{
     data_model::{prelude::*, Level},
     executor_data_model::isi::multisig::*,
 };
+use iroha_executor_data_model::permission::account::CanRegisterAccount;
 use iroha_test_network::*;
 use iroha_test_samples::{
     gen_account_in, ALICE_ID, BOB_ID, BOB_KEYPAIR, CARPENTER_ID, CARPENTER_KEYPAIR,
@@ -112,7 +113,7 @@ fn multisig_base(suite: TestSuite) -> Result<()> {
             .map(Register::account),
     )?;
 
-    let not_signatory = residents.pop_first().unwrap();
+    let non_signatory = residents.pop_first().unwrap();
     let mut signatories = residents;
 
     let register_multisig_account = MultisigRegister::new(
@@ -143,10 +144,28 @@ fn multisig_base(suite: TestSuite) -> Result<()> {
     .submit_blocking(register_multisig_account.clone())
     .expect_err("multisig account should not be registered by account of another domain");
 
-    // Any account in the same domain can register a multisig account without special permission
-    alt_client(not_signatory, &test_client)
+    // Non-signatory account in the same domain cannot register a multisig account without special permission
+    let _err = alt_client(non_signatory.clone(), &test_client)
+        .submit_blocking(register_multisig_account.clone())
+        .expect_err(
+            "multisig account should not be registered by non-signatory account of the same domain",
+        );
+
+    // All but the first signatory approve the proposal
+    let signatory = signatories.pop_first().unwrap();
+
+    // Signatory account cannot register a multisig account without special permission
+    let _err = alt_client(signatory, &test_client)
+        .submit_blocking(register_multisig_account.clone())
+        .expect_err("multisig account should not be registered by signatory account");
+
+    // Account with permission can register a multisig account
+    alt_client((BOB_ID.clone(), BOB_KEYPAIR.clone()), &test_client).submit_blocking(
+        Grant::account_permission(CanRegisterAccount { domain }, non_signatory.0.clone()),
+    )?;
+    alt_client(non_signatory, &test_client)
         .submit_blocking(register_multisig_account)
-        .expect("multisig account should be registered by account of the same domain");
+        .expect("multisig account should be registered by account with permission");
 
     // Check that the multisig account has been registered
     test_client
@@ -169,11 +188,9 @@ fn multisig_base(suite: TestSuite) -> Result<()> {
     let instructions_hash = HashOf::new(&instructions);
 
     let proposer = signatories.pop_last().unwrap();
-    // All but the first signatory approve the proposal
-    let mut approvers = signatories.into_iter().skip(1);
+    let mut approvers = signatories.into_iter();
 
     let propose = MultisigPropose::new(multisig_account_id.clone(), instructions);
-
     alt_client(proposer, &test_client).submit_blocking(propose)?;
 
     // Allow time to elapse to test the expiration
@@ -249,7 +266,7 @@ fn multisig_base(suite: TestSuite) -> Result<()> {
 ///   0       1    2   3  4  5 <--- personal signatories
 /// ```
 #[test]
-#[expect(clippy::similar_names)]
+#[expect(clippy::similar_names, clippy::too_many_lines)]
 fn multisig_recursion() -> Result<()> {
     let (network, _rt) = NetworkBuilder::new().start_blocking()?;
     let test_client = network.client();

crates/iroha_executor/src/default/isi/mod.rs

@@ -3,8 +3,6 @@ use iroha_executor_data_model::isi::multisig::MultisigInstructionBox;
 use super::*;
 use crate::prelude::{Execute, Vec, Visit};
 
-mod multisig;
-
 pub fn visit_custom_instruction<V: Execute + Visit + ?Sized>(
     executor: &mut V,
     instruction: &CustomInstruction,
@@ -18,12 +16,14 @@ pub fn visit_custom_instruction<V: Execute + Visit + ?Sized>(
 
 trait VisitExecute: crate::data_model::isi::Instruction {
     fn visit_execute<V: Execute + Visit + ?Sized>(self, executor: &mut V) {
+        let init_authority = executor.context().authority.clone();
         self.visit(executor);
         if executor.verdict().is_ok() {
             if let Err(err) = self.execute(executor) {
                 executor.deny(err);
             }
         }
+        executor.context_mut().authority = init_authority;
     }
 
     fn visit<V: Execute + Visit + ?Sized>(&self, _executor: &mut V) {
@@ -34,3 +34,14 @@ trait VisitExecute: crate::data_model::isi::Instruction {
         unimplemented!("should be overridden unless `Self::visit_execute` is overridden")
     }
 }
+
+macro_rules! visit_seq {
+    ($executor:ident.$visit:ident($instruction:expr)) => {
+        $executor.$visit($instruction);
+        if $executor.verdict().is_err() {
+            return $executor.verdict().clone();
+        }
+    };
+}
+
+mod multisig;

crates/iroha_executor/src/default/isi/multisig/account.rs

@@ -1,20 +1,37 @@
 //! Validation and execution logic of instructions for multisig accounts
 
+use iroha_executor_data_model::permission::account::CanRegisterAccount;
+
 use super::*;
+use crate::permission::domain::is_domain_owner;
 
 impl VisitExecute for MultisigRegister {
     fn visit<V: Execute + Visit + ?Sized>(&self, executor: &mut V) {
-        let host = executor.host();
+        let registrant = executor.context().authority.clone();
         let target_domain = self.account.domain();
+        let host = executor.host();
+
+        let Ok(is_domain_owner) = is_domain_owner(target_domain, &registrant, host) else {
+            deny!(
+                executor,
+                "domain must exist before registering multisig account"
+            );
+        };
 
-        // Any account in a domain can register any multisig account in the domain
-        // TODO Restrict access to the multisig signatories?
-        // TODO Impose proposal and approval process?
-        if target_domain != executor.context().authority.domain() {
+        let has_permission = {
+            CanRegisterAccount {
+                domain: target_domain.clone(),
+            }
+            .is_owned_by(&registrant, host)
+        };
+
+        // Impose the same restriction as for personal account registrations
+        // TODO Allow the signatories to register the multisig account? With propose and approve procedures?
+        if !(is_domain_owner || has_permission) {
             deny!(
                 executor,
-                "multisig account and its registrant must be in the same domain"
-            )
+                "registrant must have sufficient permission to register an account"
+            );
         }
 
         for signatory in self.signatories.keys().cloned() {
@@ -33,52 +50,56 @@ impl VisitExecute for MultisigRegister {
     }
 
     fn execute<V: Execute + Visit + ?Sized>(self, executor: &mut V) -> Result<(), ValidationFail> {
-        let host = executor.host();
+        let host = executor.host().clone();
+        let domain_owner = host
+            .query(FindDomains)
+            .filter_with(|domain| domain.id.eq(self.account.domain().clone()))
+            .execute_single()
+            .dbg_unwrap()
+            .owned_by()
+            .clone();
+
+        // Authorize as the domain owner:
+        // Just having permission to register accounts is insufficient to register multisig roles
+        executor.context_mut().authority = domain_owner.clone();
+
         let multisig_account = self.account;
         let multisig_role = multisig_role_for(&multisig_account);
 
-        host.submit(&Register::account(Account::new(multisig_account.clone())))
-            .dbg_expect("registrant should successfully register a multisig account");
+        visit_seq!(executor
+            .visit_register_account(&Register::account(Account::new(multisig_account.clone()))));
 
-        host.submit(&SetKeyValue::account(
+        visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account(
             multisig_account.clone(),
             SIGNATORIES.parse().unwrap(),
             Json::new(&self.signatories),
-        ))
-        .dbg_unwrap();
+        )));
 
-        host.submit(&SetKeyValue::account(
+        visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account(
             multisig_account.clone(),
             QUORUM.parse().unwrap(),
             Json::new(self.quorum),
-        ))
-        .dbg_unwrap();
+        )));
 
-        host.submit(&SetKeyValue::account(
+        visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account(
             multisig_account.clone(),
             TRANSACTION_TTL_MS.parse().unwrap(),
             Json::new(self.transaction_ttl_ms),
-        ))
-        .dbg_unwrap();
+        )));
 
-        host.submit(&Register::role(
-            // No use, but temporarily grant a multisig role to the multisig account due to specifications
-            Role::new(multisig_role.clone(), multisig_account.clone()),
-        ))
-        .dbg_expect("registrant should successfully register a multisig role");
+        visit_seq!(executor.visit_register_role(&Register::role(
+            // Temporarily grant a multisig role to the domain owner to delegate the role to the signatories
+            Role::new(multisig_role.clone(), domain_owner.clone()),
+        )));
 
         for signatory in self.signatories.keys().cloned() {
-            host.submit(&Grant::account_role(multisig_role.clone(), signatory))
-                .dbg_expect(
-                    "registrant should successfully grant the multisig role to signatories",
-                );
+            visit_seq!(executor
+                .visit_grant_account_role(&Grant::account_role(multisig_role.clone(), signatory)));
         }
 
-        // FIXME No roles to revoke found, which should have been granted to the multisig account
-        // host.submit(&Revoke::account_role(multisig_role, multisig_account))
-        //     .dbg_expect(
-        //         "registrant should successfully revoke the multisig role from the multisig account",
-        //     );
+        visit_seq!(
+            executor.visit_revoke_account_role(&Revoke::account_role(multisig_role, domain_owner))
+        );
 
         Ok(())
     }