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.

Sign up for GitHub

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(torii): configutation file for all torii cli options #2646

Merged
merged 8 commits into from
Nov 9, 2024
Merged

feat(torii): configutation file for all torii cli options #2646

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d6c77c0
feat(torii): configutation file for all torii cli options
Larkooo Nov 6, 2024
3108039
fix: config
Larkooo Nov 8, 2024
aa985f7
skip config
Larkooo Nov 8, 2024
7c19061
wrap up config
Larkooo Nov 8, 2024
cf20035
optional world address
Larkooo Nov 8, 2024
f671543
hisotrical event message sconfig
Larkooo Nov 8, 2024
8b41cf2
remove upgrade
Larkooo Nov 8, 2024
a2c064e
fmt clippy
Larkooo Nov 8, 2024
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
View file
Open in desktop

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

1 change: 1 addition & 0 deletions bin/torii/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ torii-grpc = { workspace = true, features = [ "server" ] }
torii-relay.workspace = true
torii-server.workspace = true
tower.workspace = true
toml.workspace = true

tower-http.workspace = true
tracing-subscriber.workspace = true
Expand Down
47 changes: 16 additions & 31 deletions bin/torii/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use clap::{ArgAction, Parser};
use dojo_metrics::exporters::prometheus::PrometheusRecorder;
use dojo_utils::parse::{parse_socket_address, parse_url};
use dojo_world::contracts::world::WorldContractReader;
use serde::{Deserialize, Serialize};
use sqlx::sqlite::{
SqliteAutoVacuum, SqliteConnectOptions, SqliteJournalMode, SqlitePoolOptions, SqliteSynchronous,
};
Expand All @@ -39,7 +40,7 @@ use torii_core::executor::Executor;
use torii_core::processors::store_transaction::StoreTransactionProcessor;
use torii_core::simple_broker::SimpleBroker;
use torii_core::sql::Sql;
use torii_core::types::{Contract, ContractType, Model, ToriiConfig};
use torii_core::types::{Contract, ContractType, Model};
use torii_server::proxy::Proxy;
use tracing::{error, info};
use tracing_subscriber::{fmt, EnvFilter};
Expand All @@ -48,7 +49,7 @@ use url::{form_urlencoded, Url};
pub(crate) const LOG_TARGET: &str = "torii::cli";

/// Dojo World Indexer
#[derive(Parser, Debug)]
#[derive(Parser, Debug, Serialize, Deserialize)]
#[command(name = "torii", author, version, about, long_about = None)]
struct Args {
/// The world to index
Expand Down Expand Up @@ -148,20 +149,24 @@ struct Args {
config: Option<PathBuf>,
}

impl Args {
fn from_file(path: &PathBuf) -> anyhow::Result<Self> {
let content = std::fs::read_to_string(path)
.with_context(|| format!("Failed to read config file: {}", path.display()))?;

toml::from_str(&content)
.with_context(|| "Failed to parse TOML config")
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ohayo sensei! Let's enhance the error handling and validation!

The from_file implementation could benefit from some improvements:

  1. Add file existence check before reading
  2. Trim whitespace from content before parsing
  3. Provide more specific error context for TOML parsing failures

Here's a suggested improvement:

 fn from_file(path: &PathBuf) -> anyhow::Result<Self> {
+    if !path.exists() {
+        anyhow::bail!("Config file not found: {}", path.display());
+    }
     let content = std::fs::read_to_string(path)
         .with_context(|| format!("Failed to read config file: {}", path.display()))?;
     
-    toml::from_str(&content)
-        .with_context(|| "Failed to parse TOML config")
+    toml::from_str(content.trim())
+        .with_context(|| format!("Failed to parse TOML config at: {}", path.display()))
 }
📝 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
fn from_file(path: &PathBuf) -> anyhow::Result<Self> {
let content = std::fs::read_to_string(path)
.with_context(|| format!("Failed to read config file: {}", path.display()))?;
toml::from_str(&content)
.with_context(|| "Failed to parse TOML config")
}
fn from_file(path: &PathBuf) -> anyhow::Result<Self> {
if !path.exists() {
anyhow::bail!("Config file not found: {}", path.display());
}
let content = std::fs::read_to_string(path)
.with_context(|| format!("Failed to read config file: {}", path.display()))?;
toml::from_str(content.trim())
.with_context(|| format!("Failed to parse TOML config at: {}", path.display()))
}

}

#[tokio::main]
async fn main() -> anyhow::Result<()> {
let args = Args::parse();

let mut config = if let Some(path) = args.config {
ToriiConfig::load_from_path(&path)?
let args = if let Some(path) = args.config {
Args::from_file(&path)?
} else {
let mut config = ToriiConfig::default();

if let Some(contracts) = args.contracts {
config.contracts = VecDeque::from(contracts);
}

config
args
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Ohayo sensei! Your configuration concerns are valid and need attention!

The codebase analysis reveals:

  • No documentation exists about the TOML config file format
  • The Args::from_file implementation simply deserializes TOML without precedence rules
  • No config examples or tests are present in the codebase
  • The torii README lacks configuration documentation

Action items:

  • Add documentation for the TOML config format matching the Args struct fields: 
    world_address = "0x..."
rpc = "http://..."
database = "path/to/db"
addr = "0.0.0.0:8080"
relay_port = 9090
  • Implement and document clear precedence rules between CLI args and config
  • Add config file examples in the documentation
  • Consider adding a --config-only flag for exclusive config file usage
🔗 Analysis chain

Ohayo sensei! Let's address some configuration concerns!

The configuration loading logic could benefit from some improvements:

  1. Consider adding a config-only mode where other CLI args are ignored
  2. Document the precedence between CLI args and config file values
  3. Add documentation about the expected TOML config file format

Let's check if there's any existing documentation about the config file format:

Consider adding a README section or documentation file explaining:

  • The TOML configuration file format
  • Examples of valid configuration
  • Precedence rules between CLI args and config file

Let me analyze the codebase more thoroughly to verify the concerns about configuration documentation and handling. The previous search was too narrow and didn't check the repository structure properly.

I need to verify how the config file is handled and if there's any existing documentation in the torii-specific README before making a final determination about the configuration concerns.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Look for config file documentation
echo "Checking for config documentation..."
rg -i "config.*file.*format|toml.*config|configuration.*file" README.md docs/

Length of output: 208

Script:

#!/bin/bash
# Let's check for any documentation about configuration in the repository
echo "Checking for any documentation files..."
fd -e md

# Look for TOML-related code and comments
echo -e "\nChecking for TOML-related code..."
rg -i "toml" --type rust

# Check the Args struct definition to understand config handling
echo -e "\nChecking Args struct definition..."
rg "struct Args" -A 20 bin/torii/src/

Length of output: 9376

Script:

#!/bin/bash
# Let's check the implementation of Args::from_file to understand config handling
echo "Checking Args::from_file implementation..."
rg -A 10 "impl.*Args" bin/torii/src/

# Look for any existing config examples or tests
echo -e "\nChecking for config examples or tests..."
rg "config.*toml" bin/torii/

# Check the bin/torii README for any documentation
echo -e "\nChecking torii README..."
cat bin/torii/README.md

Length of output: 1063

};

let world_address = verify_single_world_address(args.world_address, &mut config)?;
Expand Down Expand Up @@ -321,26 +326,6 @@ async fn main() -> anyhow::Result<()> {
Ok(())
}

// Verifies that the world address is defined at most once
// and returns the world address
fn verify_single_world_address(
world_address: Option<Felt>,
config: &mut ToriiConfig,
) -> anyhow::Result<Felt> {
let world_from_config =
config.contracts.iter().find(|c| c.r#type == ContractType::WORLD).map(|c| c.address);

match (world_address, world_from_config) {
(Some(_), Some(_)) => Err(anyhow::anyhow!("World address specified multiple times")),
(Some(addr), _) => {
config.contracts.push_front(Contract { address: addr, r#type: ContractType::WORLD });
Ok(addr)
}
(_, Some(addr)) => Ok(addr),
(None, None) => Err(anyhow::anyhow!("World address not specified")),
}
}

async fn spawn_rebuilding_graphql_server(
shutdown_tx: Sender<()>,
pool: Arc<SqlitePool>,
Expand Down
15 changes: 0 additions & 15 deletions crates/torii/core/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,21 +123,6 @@ pub struct Event {
pub executed_at: DateTime<Utc>,
pub created_at: DateTime<Utc>,
}

#[derive(Default, Deserialize, Debug, Clone)]
pub struct ToriiConfig {
/// contract addresses to index
pub contracts: VecDeque<Contract>,
}

impl ToriiConfig {
pub fn load_from_path(path: &PathBuf) -> Result<Self, anyhow::Error> {
let config = std::fs::read_to_string(path)?;
let config: Self = toml::from_str(&config)?;
Ok(config)
}
}

#[derive(Deserialize, Debug, Clone, Copy)]
pub struct Contract {
pub address: Felt,
Expand Down
Loading