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

fix: fix sozo issues with migration on sepolia #2398

Merged
merged 2 commits into from
Sep 8, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ bindings
justfile
spawn-and-move-db
types-test-db
examples/spawn-and-move/manifests/saya/**
1 change: 1 addition & 0 deletions .tool-versions
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
scarb 2.7.0
starknet-foundry 0.30.0
2 changes: 2 additions & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions bin/saya/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ version.workspace = true
anyhow.workspace = true
clap.workspace = true
console.workspace = true
dojo-utils.workspace = true
katana-primitives.workspace = true
katana-rpc-api.workspace = true
saya-core.workspace = true
Expand Down
27 changes: 25 additions & 2 deletions bin/saya/src/args/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ use std::io::BufReader;
use std::path::PathBuf;

use clap::Parser;
use dojo_utils::keystore::prompt_password_if_needed;
Copy link

Choose a reason for hiding this comment

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

Refactor suggestion: Consider modularizing keystore handling.

The import of prompt_password_if_needed directly in the global scope suggests that password prompting and keystore handling are significant aspects of this module. Consider encapsulating these functionalities into a separate module or class to improve modularity and maintainability.

use saya_core::data_availability::celestia::CelestiaConfig;
use saya_core::data_availability::DataAvailabilityConfig;
use saya_core::{ProverAccessKey, SayaConfig, StarknetAccountData};
use starknet::core::utils::cairo_short_string_to_felt;
use starknet::signers::SigningKey;
use starknet_account::StarknetAccountOptions;
use tracing::Subscriber;
use tracing_subscriber::{fmt, EnvFilter};
Expand Down Expand Up @@ -130,11 +132,30 @@ impl TryFrom<SayaArgs> for SayaConfig {
None => None,
};

// Check if the private key is from keystore or provided directly to follow `sozo`
// conventions.
let private_key = if let Some(pk) = args.starknet_account.signer_key {
pk
} else if let Some(path) = args.starknet_account.signer_keystore_path {
let password = prompt_password_if_needed(
args.starknet_account.signer_keystore_password.as_deref(),
false,
)?;

SigningKey::from_keystore(path, &password)?.secret_scalar()
} else {
return Err(Box::new(std::io::Error::new(
std::io::ErrorKind::InvalidInput,
"Could not find private key. Please specify the private key or path to the \
keystore file.",
)));
};

let starknet_account = StarknetAccountData {
starknet_url: args.starknet_account.starknet_url,
chain_id: cairo_short_string_to_felt(&args.starknet_account.chain_id)?,
signer_address: args.starknet_account.signer_address,
signer_key: args.starknet_account.signer_key,
signer_key: private_key,
};

let prover_key =
Expand Down Expand Up @@ -200,7 +221,9 @@ mod tests {
starknet_url: Url::parse("http://localhost:5030").unwrap(),
chain_id: "SN_SEPOLIA".to_string(),
signer_address: Default::default(),
signer_key: Default::default(),
signer_key: None,
signer_keystore_path: None,
signer_keystore_password: None,
},
};

Expand Down
3 changes: 2 additions & 1 deletion bin/saya/src/args/proof.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use clap::Args;
use dojo_utils::env::DOJO_WORLD_ADDRESS_ENV_VAR;
use katana_primitives::felt::FieldElement;
use url::Url;

#[derive(Debug, Args, Clone)]
pub struct ProofOptions {
#[arg(help = "The address of the World contract.")]
#[arg(long = "world")]
#[arg(long = "world", env = DOJO_WORLD_ADDRESS_ENV_VAR)]
pub world_address: FieldElement,

#[arg(help = "The address of the Fact Registry contract.")]
Expand Down
25 changes: 18 additions & 7 deletions bin/saya/src/args/starknet_account.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
//! Data availability options.

use clap::Args;
use dojo_utils::env::{
DOJO_ACCOUNT_ADDRESS_ENV_VAR, DOJO_KEYSTORE_PASSWORD_ENV_VAR, DOJO_KEYSTORE_PATH_ENV_VAR,
DOJO_PRIVATE_KEY_ENV_VAR, STARKNET_RPC_URL_ENV_VAR,
};
use katana_primitives::felt::FieldElement;
use url::Url;

#[derive(Debug, Args, Clone)]
pub struct StarknetAccountOptions {
#[arg(long)]
#[arg(env)]
#[arg(long, env = STARKNET_RPC_URL_ENV_VAR)]
#[arg(help = "The url of the starknet node.")]
pub starknet_url: Url,

Expand All @@ -16,13 +19,21 @@ pub struct StarknetAccountOptions {
#[arg(help = "The chain id of the starknet node.")]
pub chain_id: String,

#[arg(long)]
#[arg(env)]
#[arg(long, env = DOJO_ACCOUNT_ADDRESS_ENV_VAR)]
#[arg(help = "The address of the starknet account.")]
pub signer_address: FieldElement,

#[arg(long)]
#[arg(env)]
#[arg(long, env = DOJO_PRIVATE_KEY_ENV_VAR)]
#[arg(help = "The private key of the starknet account.")]
pub signer_key: FieldElement,
pub signer_key: Option<FieldElement>,

#[arg(long = "keystore", env = DOJO_KEYSTORE_PATH_ENV_VAR)]
#[arg(value_name = "PATH")]
#[arg(help = "The path to the keystore file.")]
pub signer_keystore_path: Option<String>,

#[arg(long = "password", env = DOJO_KEYSTORE_PASSWORD_ENV_VAR)]
#[arg(value_name = "PASSWORD")]
#[arg(help = "The password to the keystore file.")]
pub signer_keystore_password: Option<String>,
}
2 changes: 1 addition & 1 deletion bin/sozo/src/commands/options/account/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::str::FromStr;

use anyhow::{anyhow, Context, Result};
use clap::Args;
use dojo_utils::env::DOJO_ACCOUNT_ADDRESS_ENV_VAR;
use dojo_world::config::Environment;
use scarb::core::Config;
use starknet::accounts::{ExecutionEncoding, SingleOwnerAccount};
Expand All @@ -13,7 +14,6 @@ use url::Url;

use super::signer::SignerOptions;
use super::starknet::StarknetOptions;
use super::DOJO_ACCOUNT_ADDRESS_ENV_VAR;

#[cfg(feature = "controller")]
pub mod controller;
Expand Down
7 changes: 0 additions & 7 deletions bin/sozo/src/commands/options/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,3 @@ pub mod signer;
pub mod starknet;
pub mod transaction;
pub mod world;

const STARKNET_RPC_URL_ENV_VAR: &str = "STARKNET_RPC_URL";
const DOJO_PRIVATE_KEY_ENV_VAR: &str = "DOJO_PRIVATE_KEY";
const DOJO_KEYSTORE_PATH_ENV_VAR: &str = "DOJO_KEYSTORE_PATH";
const DOJO_KEYSTORE_PASSWORD_ENV_VAR: &str = "DOJO_KEYSTORE_PASSWORD";
const DOJO_ACCOUNT_ADDRESS_ENV_VAR: &str = "DOJO_ACCOUNT_ADDRESS";
const DOJO_WORLD_ADDRESS_ENV_VAR: &str = "DOJO_WORLD_ADDRESS";
95 changes: 64 additions & 31 deletions bin/sozo/src/commands/options/signer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ use std::str::FromStr;

use anyhow::{anyhow, Result};
use clap::Args;
use dojo_utils::env::{
DOJO_KEYSTORE_PASSWORD_ENV_VAR, DOJO_KEYSTORE_PATH_ENV_VAR, DOJO_PRIVATE_KEY_ENV_VAR,
};
use dojo_utils::keystore::prompt_password_if_needed;
use dojo_world::config::Environment;
use starknet::core::types::Felt;
use starknet::signers::{LocalWallet, SigningKey};
use tracing::trace;

use super::{DOJO_KEYSTORE_PASSWORD_ENV_VAR, DOJO_KEYSTORE_PATH_ENV_VAR, DOJO_PRIVATE_KEY_ENV_VAR};

#[derive(Debug, Args, Clone)]
#[command(next_help_heading = "Signer options")]
// INVARIANT:
Expand Down Expand Up @@ -42,35 +44,73 @@ pub struct SignerOptions {
}

impl SignerOptions {
/// Retrieves the signer from the CLI or environment metadata.
/// First, attempt to locate the signer from CLI arguments or environment variables via CLAP.
/// If unsuccessful, then search for the signer within the Dojo environment metadata.
/// If the signer is not found in any of the above locations, return an error.
pub fn signer(&self, env_metadata: Option<&Environment>, no_wait: bool) -> Result<LocalWallet> {
if let Some(private_key) = self.private_key(env_metadata) {
trace!(private_key, "Signing using private key.");
return Ok(LocalWallet::from_signing_key(SigningKey::from_secret_scalar(
Felt::from_str(&private_key)?,
)));
let pk_cli = self.private_key.clone();
let pk_env = env_metadata.and_then(|env| env.private_key().map(|s| s.to_string()));

let pk_keystore_cli = self.private_key_from_keystore_cli(no_wait)?;
let pk_keystore_env = self.private_key_from_keystore_env(env_metadata, no_wait)?;

let private_key = if let Some(private_key) = pk_cli {
trace!("Signing using private key from CLI.");
SigningKey::from_secret_scalar(Felt::from_str(&private_key)?)
} else if let Some(private_key) = pk_keystore_cli {
trace!("Signing using private key from CLI keystore.");
private_key
} else if let Some(private_key) = pk_env {
trace!("Signing using private key from env metadata.");
SigningKey::from_secret_scalar(Felt::from_str(&private_key)?)
} else if let Some(private_key) = pk_keystore_env {
trace!("Signing using private key from env metadata keystore.");
private_key
} else {
return Err(anyhow!(
"Could not find private key. Please specify the private key or path to the \
keystore file."
));
};

Ok(LocalWallet::from_signing_key(private_key))
}
Copy link

Choose a reason for hiding this comment

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

Refactor suggestion: Improve clarity and modularity in signer method.

The signer method has been significantly refactored to streamline the process of obtaining a private key from various sources. While the multi-source approach enhances robustness, the method's complexity could be reduced by further modularizing the private key retrieval logic into separate methods or even a dedicated class. This would improve readability and maintainability.


/// Retrieves the private key from the CLI keystore.
/// If the keystore path is not set, it returns `None`.
pub fn private_key_from_keystore_cli(&self, no_wait: bool) -> Result<Option<SigningKey>> {
if let Some(path) = &self.keystore_path {
let password = prompt_password_if_needed(self.keystore_password.as_deref(), no_wait)?;

let private_key = SigningKey::from_keystore(path, &password)?;
return Ok(Some(private_key));
}

if let Some(path) = self.keystore_path(env_metadata) {
let password = {
if let Some(password) = self.keystore_password(env_metadata) {
password.to_owned()
} else if no_wait {
return Err(anyhow!("Could not find password. Please specify the password."));
} else {
trace!("Prompting user for keystore password.");
rpassword::prompt_password("Enter password: ")?
}
};
Ok(None)
}

/// Retrieves the private key from the keystore in the environment metadata.
/// If the keystore path is not set, it returns `None`.
pub fn private_key_from_keystore_env(
&self,
env_metadata: Option<&Environment>,
no_wait: bool,
) -> Result<Option<SigningKey>> {
if let Some(path) = env_metadata.and_then(|env| env.keystore_path()) {
let password = prompt_password_if_needed(
env_metadata.and_then(|env| env.keystore_password()),
no_wait,
)?;

let private_key = SigningKey::from_keystore(path, &password)?;
return Ok(LocalWallet::from_signing_key(private_key));
return Ok(Some(private_key));
}

Err(anyhow!(
"Could not find private key. Please specify the private key or path to the keystore \
file."
))
Ok(None)
}

/// Retrieves the private key from the CLI or environment metadata.
pub fn private_key(&self, env_metadata: Option<&Environment>) -> Option<String> {
if let Some(s) = &self.private_key {
Some(s.to_owned())
Expand All @@ -79,21 +119,14 @@ impl SignerOptions {
}
}

/// Retrieves the keystore path from the CLI or environment metadata.
pub fn keystore_path(&self, env_metadata: Option<&Environment>) -> Option<String> {
if let Some(s) = &self.keystore_path {
Some(s.to_owned())
} else {
env_metadata.and_then(|env| env.keystore_path().map(|s| s.to_string()))
}
}

pub fn keystore_password(&self, env_metadata: Option<&Environment>) -> Option<String> {
if let Some(s) = &self.keystore_password {
Some(s.to_owned())
} else {
env_metadata.and_then(|env| env.keystore_password().map(|s| s.to_string()))
}
}
}

#[cfg(test)]
Expand Down
5 changes: 2 additions & 3 deletions bin/sozo/src/commands/options/starknet.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
use anyhow::Result;
use clap::Args;
use dojo_utils::env::STARKNET_RPC_URL_ENV_VAR;
use dojo_world::config::Environment;
use starknet::providers::jsonrpc::HttpTransport;
use starknet::providers::JsonRpcClient;
use tracing::trace;
use url::Url;

use super::STARKNET_RPC_URL_ENV_VAR;

#[derive(Debug, Args, Clone)]
#[command(next_help_heading = "Starknet options")]
pub struct StarknetOptions {
Expand Down Expand Up @@ -49,9 +48,9 @@ impl StarknetOptions {
#[cfg(test)]
mod tests {
use clap::Parser;
use dojo_utils::env::STARKNET_RPC_URL_ENV_VAR;

use super::StarknetOptions;
use crate::commands::options::STARKNET_RPC_URL_ENV_VAR;

const ENV_RPC: &str = "http://localhost:7474/";
const METADATA_RPC: &str = "http://localhost:6060/";
Expand Down
3 changes: 1 addition & 2 deletions bin/sozo/src/commands/options/world.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@ use std::str::FromStr;

use anyhow::{anyhow, Result};
use clap::Args;
use dojo_utils::env::DOJO_WORLD_ADDRESS_ENV_VAR;
use dojo_world::config::Environment;
use starknet::core::types::Felt;
use tracing::trace;

use super::DOJO_WORLD_ADDRESS_ENV_VAR;

#[derive(Debug, Args, Clone)]
#[command(next_help_heading = "World options")]
pub struct WorldOptions {
Expand Down
2 changes: 1 addition & 1 deletion crates/dojo-bindgen/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ fn gather_dojo_data(
let mut base_manifest = BaseManifest::load_from_path(&base_manifest_dir)?;

if let Some(skip_manifests) = skip_migration {
base_manifest.remove_tags(skip_manifests);
base_manifest.remove_tags(&skip_manifests);
}

let mut models = HashMap::new();
Expand Down
2 changes: 1 addition & 1 deletion crates/dojo-test-utils/src/migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ pub fn prepare_migration(
)
.unwrap();

if let Some(skip_manifests) = skip_migration {
if let Some(skip_manifests) = &skip_migration {
manifest.remove_tags(skip_manifests);
}

Expand Down
1 change: 1 addition & 0 deletions crates/dojo-utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ futures.workspace = true
reqwest.workspace = true
starknet.workspace = true
thiserror.workspace = true
rpassword.workspace = true
tokio = { version = "1", features = [ "time" ], default-features = false }

[dev-dependencies]
Expand Down
6 changes: 6 additions & 0 deletions crates/dojo-utils/src/env.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pub const STARKNET_RPC_URL_ENV_VAR: &str = "STARKNET_RPC_URL";
pub const DOJO_PRIVATE_KEY_ENV_VAR: &str = "DOJO_PRIVATE_KEY";
pub const DOJO_KEYSTORE_PATH_ENV_VAR: &str = "DOJO_KEYSTORE_PATH";
pub const DOJO_KEYSTORE_PASSWORD_ENV_VAR: &str = "DOJO_KEYSTORE_PASSWORD";
pub const DOJO_ACCOUNT_ADDRESS_ENV_VAR: &str = "DOJO_ACCOUNT_ADDRESS";
pub const DOJO_WORLD_ADDRESS_ENV_VAR: &str = "DOJO_WORLD_ADDRESS";
13 changes: 13 additions & 0 deletions crates/dojo-utils/src/keystore.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
use anyhow::{anyhow, Result};

/// Prompts the user for a password if no password is provided and `no_wait` is not set.
/// The `no_wait` is used for non-interactive commands.
pub fn prompt_password_if_needed(maybe_password: Option<&str>, no_wait: bool) -> Result<String> {
if let Some(password) = maybe_password {
Ok(password.to_owned())
} else if no_wait {
Err(anyhow!("Could not find password. Please specify the password."))
} else {
Ok(rpassword::prompt_password("Enter password: ")?.to_owned())
}
}
Loading
Loading