review: address up to #issuecomment-2453963837

Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com>

crates/iroha/tests/multisig.rs

@@ -11,7 +11,6 @@ use iroha::{
     data_model::{prelude::*, Level},
     multisig_data_model::*,
 };
-use iroha_multisig_data_model::approvals_key;
 use iroha_test_network::*;
 use iroha_test_samples::{
     gen_account_in, ALICE_ID, BOB_ID, BOB_KEYPAIR, CARPENTER_ID, CARPENTER_KEYPAIR,
@@ -79,7 +78,7 @@ fn multisig_expires() -> Result<()> {
 ///     - Transaction has not expired
 ///     - Every instruction validated against the multisig account: authorized
 /// 6. Either execution or expiration on approval deletes the transaction entry
-#[expect(clippy::cast_possible_truncation)]
+#[expect(clippy::cast_possible_truncation, clippy::too_many_lines)]
 fn multisig_base(suite: TestSuite) -> Result<()> {
     const N_SIGNATORIES: usize = 5;
 
@@ -174,7 +173,7 @@ fn multisig_base(suite: TestSuite) -> Result<()> {
     };
     test_client.submit_blocking(Log::new(Level::DEBUG, "Just ticking time".to_string()))?;
 
-    let approve = MultisigApprove::new(multisig_account_id.clone(), instructions_hash.clone());
+    let approve = MultisigApprove::new(multisig_account_id.clone(), instructions_hash);
 
     // Approve once to see if the proposal expires
     let approver = approvers.next().unwrap();
@@ -216,7 +215,9 @@ fn multisig_base(suite: TestSuite) -> Result<()> {
     // Check if the transaction entry is deleted
     let res = test_client.query_single(FindAccountMetadata::new(
         multisig_account_id,
-        instructions_key(&instructions_hash),
+        format!("proposals/{instructions_hash}/instructions")
+            .parse()
+            .unwrap(),
     ));
     match (&transaction_ttl_ms_opt, &unauthorized_target_opt) {
         // In case failing validation, the entry can exit only by expiring
@@ -335,7 +336,9 @@ fn multisig_recursion() -> Result<()> {
     let approvals_at_12: BTreeSet<AccountId> = test_client
         .query_single(FindAccountMetadata::new(
             msa_12.clone(),
-            approvals_key(&approval_hash_to_12345),
+            format!("proposals/{approval_hash_to_12345}/approvals")
+                .parse()
+                .unwrap(),
         ))
         .expect("leaf approvals should be initialized by the root proposal")
         .try_into_any()
@@ -381,7 +384,13 @@ fn reserved_names() {
 
     {
         let register = {
-            let role = multisig_role_for(&account_in_another_domain);
+            let role = format!(
+                "MULTISIG_SIGNATORY/{}/{}",
+                account_in_another_domain.domain(),
+                account_in_another_domain.signatory()
+            )
+            .parse()
+            .unwrap();
             Register::role(Role::new(role, ALICE_ID.clone()))
         };
         let _err = test_client.submit_blocking(register).expect_err(

crates/iroha_cli/src/main.rs

@@ -1309,6 +1309,22 @@ mod multisig {
         }
     }
 
+    const DELIMITER: char = '/';
+    const PROPOSALS: &str = "proposals";
+    const MULTISIG_SIGNATORY: &str = "MULTISIG_SIGNATORY";
+
+    fn multisig_account_from(role: &RoleId) -> Option<AccountId> {
+        role.name()
+            .as_ref()
+            .strip_prefix(MULTISIG_SIGNATORY)?
+            .rsplit_once(DELIMITER)
+            .and_then(|(init, last)| {
+                format!("{last}@{}", init.trim_matches(DELIMITER))
+                    .parse()
+                    .ok()
+            })
+    }
+
     /// Recursively trace back to the root multisig account
     fn trace_back_from(
         account: AccountId,
@@ -1317,7 +1333,7 @@ mod multisig {
     ) -> Result<()> {
         let Ok(multisig_roles) = client
             .query(FindRolesByAccountId::new(account))
-            .filter_with(|role_id| role_id.name.starts_with(MULTISIG_SIGNATORY_))
+            .filter_with(|role_id| role_id.name.starts_with(MULTISIG_SIGNATORY))
             .execute_all()
         else {
             return Ok(());

crates/iroha_executor/src/default.rs

@@ -1162,7 +1162,6 @@ pub mod parameter {
 
 pub mod role {
     use iroha_executor_data_model::permission::role::CanManageRoles;
-    use iroha_multisig_data_model::multisig_account_from;
     use iroha_smart_contract::{data_model::role::Role, Iroha};
 
     use super::*;
@@ -1229,26 +1228,18 @@ pub mod role {
         executor: &mut V,
         isi: &Register<Role>,
     ) {
+        const MULTISIG_SIGNATORY: &str = "MULTISIG_SIGNATORY";
+
         let role = isi.object();
         let mut new_role = Role::new(role.id().clone(), role.grant_to().clone());
 
         // Exception for multisig roles
-        let mut is_multisig_role = false;
-        if let Some(multisig_account) = multisig_account_from(role.id()) {
-            if crate::permission::domain::is_domain_owner(
-                multisig_account.domain(),
-                &executor.context().authority,
-                executor.host(),
+        if role.id().name().as_ref().starts_with(MULTISIG_SIGNATORY) {
+            // Multisig roles should only automatically be registered bypassing validation
+            deny!(
+                executor,
+                "role name conflicts with the reserved multisig role names"
             )
-            .unwrap_or_default()
-            {
-                is_multisig_role = true;
-            } else {
-                deny!(
-                    executor,
-                    "role name conflicts with the reserved multisig role names"
-                )
-            }
         }
 
         for permission in role.inner().permissions() {
@@ -1277,7 +1268,6 @@ pub mod role {
 
         if executor.context().curr_block.is_genesis()
             || CanManageRoles.is_owned_by(&executor.context().authority, executor.host())
-            || is_multisig_role
         {
             let grant_role = &Grant::account_role(role.id().clone(), role.grant_to().clone());
             let isi = &Register::role(new_role);

crates/iroha_genesis/src/lib.rs

@@ -19,21 +19,6 @@ use serde::{Deserialize, Serialize};
 /// Domain of the genesis account, technically required for the pre-genesis state
 pub static GENESIS_DOMAIN_ID: LazyLock<DomainId> = LazyLock::new(|| "genesis".parse().unwrap());
 
-/// Domain of the system account, implicitly registered in the genesis
-pub static SYSTEM_DOMAIN_ID: LazyLock<DomainId> = LazyLock::new(|| "system".parse().unwrap());
-
-/// The root authority for internal operations, implicitly registered in the genesis
-// FIXME #5022 deny external access
-// kagami crypto --seed "system"
-pub static SYSTEM_ACCOUNT_ID: LazyLock<AccountId> = LazyLock::new(|| {
-    AccountId::new(
-        SYSTEM_DOMAIN_ID.clone(),
-        "ed0120D8B64D62FD8E09B9F29FE04D9C63E312EFB1CB29F1BF6AF00EBC263007AE75F7"
-            .parse()
-            .unwrap(),
-    )
-});
-
 /// Genesis block.
 ///
 /// First transaction must contain single [`Upgrade`] instruction to set executor.
@@ -246,26 +231,6 @@ impl GenesisBuilder {
         }
     }
 
-    /// Entry system entities to serve standard functionality.
-    pub fn install_libs(self) -> Self {
-        let instructions = vec![
-            Register::domain(Domain::new(SYSTEM_DOMAIN_ID.clone())).into(),
-            Register::account(Account::new(SYSTEM_ACCOUNT_ID.clone())).into(),
-        ];
-
-        let wasm_triggers = vec![];
-
-        Self {
-            chain: self.chain,
-            executor: self.executor,
-            parameters: self.parameters,
-            instructions,
-            wasm_dir: self.wasm_dir,
-            wasm_triggers,
-            topology: self.topology,
-        }
-    }
-
     /// Entry a domain registration and transition to [`GenesisDomainBuilder`].
     pub fn domain(self, domain_name: Name) -> GenesisDomainBuilder {
         self.domain_with_metadata(domain_name, Metadata::default())

crates/iroha_kagami/src/genesis/generate.rs

@@ -64,7 +64,7 @@ impl<T: Write> RunArgs<T> for Args {
         } = self;
 
         let chain = ChainId::from("00000000-0000-0000-0000-000000000000");
-        let builder = GenesisBuilder::new(chain, executor, wasm_dir).install_libs();
+        let builder = GenesisBuilder::new(chain, executor, wasm_dir);
         let genesis = match mode.unwrap_or_default() {
             Mode::Default => generate_default(builder, genesis_public_key),
             Mode::Synthetic {

crates/iroha_multisig_data_model/src/lib.rs

@@ -94,67 +94,19 @@ macro_rules! impl_custom_instruction {
             fn try_from(payload: &Json) -> serde_json::Result<Self> {
                 serde_json::from_str::<Self>(payload.as_ref())
             }
-        }
+        } $(
 
-        $(
-            impl Instruction for $instruction {}
+        impl Instruction for $instruction {}
 
-            impl From<$instruction> for InstructionBox {
-                fn from(value: $instruction) -> Self {
-                    Self::Custom(<$box>::from(value).into())
-                }
+        impl From<$instruction> for InstructionBox {
+            fn from(value: $instruction) -> Self {
+                Self::Custom(<$box>::from(value).into())
             }
-        )+
+        })+
     };
 }
 
 impl_custom_instruction!(
     MultisigInstructionBox,
     MultisigRegister | MultisigPropose | MultisigApprove
 );
-
-#[allow(missing_docs)]
-mod names {
-    use super::*;
-
-    pub const SIGNATORIES: &str = "signatories";
-    pub const QUORUM: &str = "quorum";
-    pub const TRANSACTION_TTL_MS: &str = "transaction_ttl_ms";
-    pub const PROPOSALS: &str = "proposals";
-    pub const MULTISIG_SIGNATORY_: &str = "MULTISIG_SIGNATORY_";
-
-    pub fn instructions_key(hash: &HashOf<Vec<InstructionBox>>) -> Name {
-        format!("{PROPOSALS}/{hash}/instructions").parse().unwrap()
-    }
-
-    pub fn proposed_at_ms_key(hash: &HashOf<Vec<InstructionBox>>) -> Name {
-        format!("{PROPOSALS}/{hash}/proposed_at_ms")
-            .parse()
-            .unwrap()
-    }
-
-    pub fn approvals_key(hash: &HashOf<Vec<InstructionBox>>) -> Name {
-        format!("{PROPOSALS}/{hash}/approvals").parse().unwrap()
-    }
-
-    pub fn multisig_role_for(account: &AccountId) -> RoleId {
-        format!(
-            "{MULTISIG_SIGNATORY_}{}_{}",
-            account.signatory(),
-            account.domain()
-        )
-        .parse()
-        .unwrap()
-    }
-
-    pub fn multisig_account_from(role: &RoleId) -> Option<AccountId> {
-        role.name()
-            .as_ref()
-            .strip_prefix(MULTISIG_SIGNATORY_)?
-            .replacen('_', "@", 1)
-            .parse()
-            .ok()
-    }
-}
-
-pub use names::*;

crates/iroha_schema_gen/src/lib.rs

@@ -94,9 +94,6 @@ pub fn build_schemas() -> MetaMap {
 
         // Multi-signature operations
         multisig::MultisigInstructionBox,
-        multisig::MultisigRegister,
-        multisig::MultisigPropose,
-        multisig::MultisigApprove,
 
         // Genesis file - used by SDKs to generate the genesis block
         // TODO: IMO it could/should be removed from the schema

crates/iroha_telemetry_derive/Cargo.toml

@@ -30,6 +30,4 @@ manyhow = { workspace = true }
 iroha_macro_utils = { workspace = true }
 
 [dev-dependencies]
-iroha_core = { workspace = true }
-
 trybuild = { workspace = true }

defaults/genesis.json

@@ -24,23 +24,6 @@
     }
   },
   "instructions": [
-    {
-      "Register": {
-        "Domain": {
-          "id": "system",
-          "logo": null,
-          "metadata": {}
-        }
-      }
-    },
-    {
-      "Register": {
-        "Account": {
-          "id": "ed0120D8B64D62FD8E09B9F29FE04D9C63E312EFB1CB29F1BF6AF00EBC263007AE75F7@system",
-          "metadata": {}
-        }
-      }
-    },
     {
       "Register": {
         "Domain": {

wasm/libs/default_executor/src/multisig/mod.rs

@@ -1,3 +1,5 @@
+use alloc::format;
+
 use super::*;
 
 mod account;
@@ -13,4 +15,39 @@ impl VisitExecute for MultisigInstructionBox {
     }
 }
 
+const DELIMITER: char = '/';
+const SIGNATORIES: &str = "signatories";
+const QUORUM: &str = "quorum";
+const TRANSACTION_TTL_MS: &str = "transaction_ttl_ms";
+const PROPOSALS: &str = "proposals";
+const MULTISIG_SIGNATORY: &str = "MULTISIG_SIGNATORY";
+
+fn instructions_key(hash: &HashOf<Vec<InstructionBox>>) -> Name {
+    format!("{PROPOSALS}{DELIMITER}{hash}{DELIMITER}instructions")
+        .parse()
+        .unwrap()
+}
+
+fn proposed_at_ms_key(hash: &HashOf<Vec<InstructionBox>>) -> Name {
+    format!("{PROPOSALS}{DELIMITER}{hash}{DELIMITER}proposed_at_ms")
+        .parse()
+        .unwrap()
+}
+
+fn approvals_key(hash: &HashOf<Vec<InstructionBox>>) -> Name {
+    format!("{PROPOSALS}{DELIMITER}{hash}{DELIMITER}approvals")
+        .parse()
+        .unwrap()
+}
+
+fn multisig_role_for(account: &AccountId) -> RoleId {
+    format!(
+        "{MULTISIG_SIGNATORY}{DELIMITER}{}{DELIMITER}{}",
+        account.domain(),
+        account.signatory(),
+    )
+    .parse()
+    .unwrap()
+}
+
 pub(super) use iroha_multisig_data_model::*;