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 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 .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::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 @@
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,
)?;

Check warning on line 143 in bin/saya/src/args/mod.rs

View check run for this annotation

Codecov / codecov/patch

bin/saya/src/args/mod.rs#L137-L143

Added lines #L137 - L143 were not covered by tests

SigningKey::from_keystore(path, &password)?.secret_scalar()

Check warning on line 145 in bin/saya/src/args/mod.rs

View check run for this annotation

Codecov / codecov/patch

bin/saya/src/args/mod.rs#L145

Added line #L145 was not covered by tests
} 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.",
)));

Check warning on line 151 in bin/saya/src/args/mod.rs

View check run for this annotation

Codecov / codecov/patch

bin/saya/src/args/mod.rs#L147-L151

Added lines #L147 - L151 were not covered by tests
};

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,

Check warning on line 158 in bin/saya/src/args/mod.rs

View check run for this annotation

Codecov / codecov/patch

bin/saya/src/args/mod.rs#L158

Added line #L158 was not covered by tests
};

let prover_key =
Expand Down Expand Up @@ -200,7 +221,9 @@
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";
106 changes: 76 additions & 30 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 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,86 @@
}

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(env_metadata, 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))
}
Comment on lines +47 to +78
Copy link

Choose a reason for hiding this comment

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

Refactor and enhance error handling in signer method.

The signer method has been refactored to streamline the process of obtaining a private key from various sources. This multi-source approach is robust, but the complexity could be reduced by further modularizing the private key retrieval logic into separate methods, which has been done to some extent with private_key_from_keystore_cli and private_key_from_keystore_env.

However, the error message in lines 71-73 could be more specific about which sources were checked. This would aid in debugging and provide clearer feedback to the user.

Consider enhancing the error message to specify which sources were checked before failing, as follows:

- "Could not find private key. Please specify the private key or path to the keystore file."
+ "Could not find private key in CLI, environment metadata, or associated keystores. Please check your configurations."
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// 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(env_metadata, 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))
}
/// 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> {
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(env_metadata, 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 in CLI, environment metadata, or associated keystores. Please check your configurations."
));
};
Ok(LocalWallet::from_signing_key(private_key))
}


/// 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,
env_metadata: Option<&Environment>,
no_wait: bool,
) -> Result<Option<SigningKey>> {
if let Some(path) = &self.keystore_path {
let maybe_password = if self.keystore_password.is_some() {
self.keystore_password.as_deref()
} else {
env_metadata.and_then(|env| env.keystore_password())
};

let password = prompt_password_if_needed(maybe_password, 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)
}
Comment on lines +80 to +101
Copy link

Choose a reason for hiding this comment

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

Review of private_key_from_keystore_cli method.

This method effectively encapsulates the logic for fetching private keys from the CLI keystore. The use of prompt_password_if_needed for handling passwords is a good practice as it enhances security by not persisting passwords longer than necessary.

However, consider handling the scenario where the keystore path is provided but the file does not exist or is not accessible. This could improve the robustness of the method by providing clearer feedback to the user when errors occur.

Add error handling for missing or inaccessible keystore files:

+ if !Path::new(path).exists() {
+     return Err(anyhow!("Keystore file not found at the specified path: {}", path));
+ }

Committable suggestion was skipped due to low confidence.


/// 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 maybe_password = if self.keystore_password.is_some() {
self.keystore_password.as_deref()
} else {
env_metadata.and_then(|env| env.keystore_password())

Check warning on line 114 in bin/sozo/src/commands/options/signer.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/options/signer.rs#L114

Added line #L114 was not covered by tests
};

let password = prompt_password_if_needed(maybe_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)
Comment on lines +103 to +123
Copy link

Choose a reason for hiding this comment

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

Review of private_key_from_keystore_env method.

Similar to the CLI keystore method, this function handles the retrieval of private keys from the environment metadata keystore. The structure and error handling are consistent with the CLI method, which is good for maintainability.

As with the CLI method, adding checks for file existence and accessibility could prevent runtime errors and improve user experience by providing immediate feedback on misconfigurations.

Implement similar error handling as suggested for the CLI keystore method:

+ if !Path::new(path).exists() {
+     return Err(anyhow!("Keystore file not found at the specified path: {}", path));
+ }

Committable suggestion was skipped due to low confidence.

}

/// 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 +132,14 @@
}
}

/// 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 @@
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);

Check warning on line 123 in crates/dojo-bindgen/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo-bindgen/src/lib.rs#L123

Added line #L123 was not covered by tests
}

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 @@
)
.unwrap();

if let Some(skip_manifests) = skip_migration {
if let Some(skip_manifests) = &skip_migration {

Check warning on line 39 in crates/dojo-test-utils/src/migration.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo-test-utils/src/migration.rs#L39

Added line #L39 was not covered by tests
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";
Loading
Loading