Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: improve multisig utility and usability #5027

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

568 changes: 454 additions & 114 deletions crates/iroha/tests/integration/multisig.rs

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions crates/iroha_cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ path = "src/main.rs"
iroha = { workspace = true }
iroha_primitives = { workspace = true }
iroha_config_base = { workspace = true }
executor_custom_data_model = { version = "=2.0.0-rc.1.0", path = "../../wasm_samples/executor_custom_data_model" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't make sense that this is a dependency of CLI because executor_custom_data_model is intended to only be used in tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should create a separate crate for MST data model

Copy link
Contributor Author

@s8sato s8sato Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a temporary workaround. I'm planning to move those *Args for ExecuteTrigger to somewhere under the regular iroha_data_model when iroha introduces custom instructions, then CLI will only depend on iroha.

Or should we pack everything relevant to multisig to one crate for more clear modularity? Related to this, it might be better to move wasm_samples/multisig_* to somewhere as they are not just samples


thiserror = { workspace = true }
error-stack = { workspace = true, features = ["eyre"] }
Expand Down
231 changes: 230 additions & 1 deletion crates/iroha_cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ enum Subcommand {
Blocks(blocks::Args),
/// The subcommand related to multi-instructions as Json or Json5
Json(json::Args),
/// The subcommand related to multisig accounts and transactions
#[clap(subcommand)]
Multisig(multisig::Args),
}

/// Context inside which command is executed
Expand Down Expand Up @@ -165,7 +168,7 @@ macro_rules! match_all {
impl RunArgs for Subcommand {
fn run(self, context: &mut dyn RunContext) -> Result<()> {
use Subcommand::*;
match_all!((self, context), { Domain, Account, Asset, Peer, Events, Wasm, Blocks, Json })
match_all!((self, context), { Domain, Account, Asset, Peer, Events, Wasm, Blocks, Json, Multisig })
}
}

Expand Down Expand Up @@ -1197,6 +1200,232 @@ mod json {
}
}
}

mod multisig {
use std::io::{BufReader, Read as _};

use executor_custom_data_model::multisig::{MultisigAccountArgs, MultisigTransactionArgs};

use super::*;

/// Arguments for multisig subcommand
#[derive(Debug, clap::Subcommand)]
pub enum Args {
/// Register a multisig account
Register(Register),
/// Propose a multisig transaction
Propose(Propose),
/// Approve a multisig transaction
Approve(Approve),
/// List pending multisig transactions relevant to you
#[clap(subcommand)]
List(List),
}

impl RunArgs for Args {
fn run(self, context: &mut dyn RunContext) -> Result<()> {
match_all!((self, context), { Args::Register, Args::Propose, Args::Approve, Args::List })
}
}
/// Args to register a multisig account
#[derive(Debug, clap::Args)]
pub struct Register {
/// ID of the multisig account to be registered
#[arg(short, long)]
pub account: AccountId,
/// Signatories of the multisig account
#[arg(short, long, num_args(2..))]
pub signatories: Vec<AccountId>,
/// Relative weights of responsibility of respective signatories
#[arg(short, long, num_args(2..))]
pub weights: Vec<u8>,
/// Threshold of total weight at which the multisig is considered authenticated
#[arg(short, long)]
pub quorum: u16,
/// Time-to-live of multisig transactions made by the multisig account
#[arg(short, long)]
pub transaction_ttl_secs: Option<u32>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub transaction_ttl_secs: Option<u32>,
pub transaction_ttl: Duration,

why is it optional? and why not use Duration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, instead of being optional, its default value can be explicitly specified in the arg attribute.

u32 because it needs to be parsed from stdin

}

impl RunArgs for Register {
fn run(self, context: &mut dyn RunContext) -> Result<()> {
let Self {
account,
signatories,
weights,
quorum,
transaction_ttl_secs,
} = self;
if signatories.len() != weights.len() {
return Err(eyre!("signatories and weights must be equal in length"));
}
let registry_id: TriggerId = format!("multisig_accounts_{}", account.domain())
.parse()
.unwrap();
let account = account.signatory.clone();
let signatories = signatories.into_iter().zip(weights).collect();
let args = MultisigAccountArgs {
account,
signatories,
quorum,
transaction_ttl_secs,
};
let register_multisig_account =
iroha::data_model::isi::ExecuteTrigger::new(registry_id).with_args(&args);
Comment on lines +1273 to +1274
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the trigger has to already exist before executing this. Who should register it? Shouldn't it first be registered here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The domains initializer should register the accounts registry on the domain creation. See diagram in review notes


submit([register_multisig_account], Metadata::default(), context)
.wrap_err("Failed to register multisig account")
}
}

/// Args to propose a multisig transaction
#[derive(Debug, clap::Args)]
pub struct Propose {
/// Multisig authority of the multisig transaction
#[arg(short, long)]
pub account: AccountId,
}

impl RunArgs for Propose {
fn run(self, context: &mut dyn RunContext) -> Result<()> {
let Self { account } = self;
let registry_id: TriggerId = format!(
"multisig_transactions_{}_{}",
account.signatory(),
account.domain()
)
.parse()
.unwrap();
let instructions: Vec<InstructionBox> = {
let mut reader = BufReader::new(stdin());
let mut raw_content = Vec::new();
reader.read_to_end(&mut raw_content)?;
let string_content = String::from_utf8(raw_content)?;
json5::from_str(&string_content)?
};
let instructions_hash = HashOf::new(&instructions);
println!("{instructions_hash}");
let args = MultisigTransactionArgs::Propose(instructions);
let propose_multisig_transaction =
iroha::data_model::isi::ExecuteTrigger::new(registry_id).with_args(&args);
Comment on lines +1309 to +1310
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this trigger is registered by the trigger from the Register CLI subcommand

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the accounts registry should register the transactions registry on the multisig account registration. See diagram in review notes


submit([propose_multisig_transaction], Metadata::default(), context)
.wrap_err("Failed to propose transaction")
}
}

/// Args to approve a multisig transaction
#[derive(Debug, clap::Args)]
pub struct Approve {
/// Multisig authority of the multisig transaction
#[arg(short, long)]
pub account: AccountId,
/// Instructions to approve
#[arg(short, long)]
pub instructions_hash: iroha::crypto::Hash,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub instructions_hash: iroha::crypto::Hash,
pub instructions: iroha::crypto::HashOf<Vec<Instruction>>,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just utilized iroha::crypto::Hash: FromStr. It is immediately converted to HashOf<Vec<InstructionBox>>

}

impl RunArgs for Approve {
fn run(self, context: &mut dyn RunContext) -> Result<()> {
let Self {
account,
instructions_hash,
} = self;
let registry_id: TriggerId = format!(
"multisig_transactions_{}_{}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a bit annoying that we don't have any mechanism for scoping triggers to domains or accounts. I think we should be able to register triggers under accounts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current flatten triggers looks good to me. Isn't the scoping feasible by permission of the trigger authority?

account.signatory(),
account.domain()
)
.parse()
.unwrap();
let instructions_hash = HashOf::from_untyped_unchecked(instructions_hash);
let args = MultisigTransactionArgs::Approve(instructions_hash);
let approve_multisig_transaction =
iroha::data_model::isi::ExecuteTrigger::new(registry_id).with_args(&args);

submit([approve_multisig_transaction], Metadata::default(), context)
.wrap_err("Failed to approve transaction")
}
}

/// List pending multisig transactions relevant to you
#[derive(clap::Subcommand, Debug, Clone)]
pub enum List {
/// All pending multisig transactions relevant to you
All,
}

impl RunArgs for List {
fn run(self, context: &mut dyn RunContext) -> Result<()> {
let client = context.client_from_config();
let me = client.account.clone();

trace_back_from(me, &client, context)
}
}

/// Recursively trace back to the root multisig account
fn trace_back_from(
account: AccountId,
client: &Client,
context: &mut dyn RunContext,
) -> Result<()> {
let Ok(multisig_roles) = client
.query(FindRolesByAccountId::new(account))
.filter_with(|role_id| role_id.name.starts_with("multisig_signatory_"))
.execute_all()
else {
return Ok(());
};

for role_id in multisig_roles {
let super_account: AccountId = role_id
.name
.as_ref()
.strip_prefix("multisig_signatory_")
.unwrap()
.replace('_', "@")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_ is not a forbidden character in account or domain names and in fact I'd expect it to be used often

.parse()
.unwrap();

trace_back_from(super_account, client, context)?;

let transactions_registry_id: TriggerId = role_id
.name
.as_ref()
.replace("signatory", "transactions")
.parse()
.unwrap();

context.print_data(&transactions_registry_id)?;

let transactions_registry = client
.query(FindTriggers::new())
.filter_with(|trigger| trigger.id.eq(transactions_registry_id.clone()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.filter_with(|trigger| trigger.id.eq(transactions_registry_id.clone()))
.filter_with(|trigger| trigger.id.eq(transactions_registry_id))

.execute_single()?;
let proposal_kvs = transactions_registry
.action()
.metadata()
.iter()
.filter(|kv| kv.0.as_ref().starts_with("proposals"));

proposal_kvs.fold("", |acc, (k, v)| {
let mut path = k.as_ref().split('/');
let hash = path.nth(1).unwrap();

if acc != hash {
context.print_data(&hash).unwrap();
}
path.for_each(|seg| context.print_data(&seg).unwrap());
context.print_data(&v).unwrap();

hash
});
}

Ok(())
}
}
#[cfg(test)]
mod tests {
use super::*;
Expand Down
4 changes: 2 additions & 2 deletions crates/iroha_data_model/src/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -459,9 +459,9 @@ mod candidate {
);
};

if transactions.len() > 4 {
if transactions.len() > 5 {
return Err(
"Genesis block must have 1 to 4 transactions (executor upgrade, initial topology, parameters, other isi)",
"Genesis block must have 1 to 5 transactions (executor upgrade, initial topology, parameters, other instructions, trigger registrations)",
Copy link
Contributor

@mversic mversic Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Genesis block must have 1 to 5 transactions (executor upgrade, initial topology, parameters, other instructions, trigger registrations)",
"Genesis block must have 1 to 5 transactions (executor upgrade, parameters, other instructions, triggers, initial topology)",

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should respect this order in genesis

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

);
}

Expand Down
2 changes: 1 addition & 1 deletion crates/iroha_data_model/src/isi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -884,7 +884,7 @@ mod transparent {

impl Revoke<RoleId, Account> {
/// Constructs a new [`Revoke`] for a [`Role`].
pub fn role(role_id: RoleId, from: AccountId) -> Self {
pub fn account_role(role_id: RoleId, from: AccountId) -> Self {
Self {
object: role_id,
destination: from,
Expand Down
Loading
Loading