refactor: rework torii CLI to be similar to katana #2674
Conversation
WalkthroughOhayo, sensei! This pull request introduces several updates across multiple files, primarily focusing on enhancing configuration management for server options and command-line argument parsing. Key changes include the addition of default functions for server parameters, a restructuring of command-line argument handling in the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
crates/torii/core/src/types.rs (1)
124-124: Ohayo sensei! The PartialEq addition looks good!Adding
PartialEqtoContractis a sensible enhancement, especially since both of its fields (FeltandContractType) already implement this trait. This will enable proper equality comparisons ofContractinstances, which is valuable for the CLI refactoring.Consider also adding
Eqsince your type semantically supports exact equality (both field types implementEq). This would enable usingContractas a key inHashMaps and other collections that requireEq.-#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq)] +#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq)]bin/katana/src/cli/options.rs (2)
66-66: Ohayo! The ServerOptions changes look good, sensei! 🎉The changes improve configuration consistency between CLI and config file parsing. The renaming of the CORS option to
http.cors_originsenhances clarity.Consider adding a doc comment for the
http_cors_originsfield to explain the expected format of the origins (e.g., whether wildcards are supported).Also applies to: 72-72, 76-76, 83-83
279-289: Ohayo! The default functions implementation is clean, sensei! ✨The functions are well-organized and properly use the constants from the rpc config module. This ensures consistency across the codebase.
Consider adding doc comments to these default functions to explain their purpose and relationship with the configuration system, similar to other default functions in the file.
bin/torii/src/options.rs (3)
46-52: Ohayo, sensei! Correct the help message forwebsocket_port.The help message for
websocket_portincorrectly states "Port to serve Libp2p WebRTC transport." It should be updated to reflect that it serves the WebSocket transport, not WebRTC.Apply this diff to fix the help message:
help = "Port to serve Libp2p WebRTC transport." + help = "Port to serve Libp2p WebSocket transport."
96-97: Ohayo, sensei! Correct the typo in the help message forblocks_chunk_size.The word "commiting" should be spelled "committing" in the help message.
Apply this diff to fix the typo:
#[arg(long = "indexing.blocks_chunk_size", default_value_t = DEFAULT_BLOCKS_CHUNK_SIZE, help = "Number of blocks to process before commiting to DB.")] + #[arg(long = "indexing.blocks_chunk_size", default_value_t = DEFAULT_BLOCKS_CHUNK_SIZE, help = "Number of blocks to process before committing to DB.")]
212-215: Ohayo, sensei! Clarify the metrics collection behavior.The comment indicates that metrics are still collected even if the
--metricsflag is not set, and that the flag only controls whether the metrics server is started. This might be confusing to users who might expect that metrics are not collected at all when the flag is absent.Consider adjusting the implementation or updating the documentation to make this behavior clearer to users. For example, you could modify the flag to control both metrics collection and serving, or rename the flag to something like
--metrics-serverto indicate it only starts the server.bin/torii/src/main.rs (4)
145-147: Ohayo sensei! Improve merging logic forMetricsOptionsComparing entire structs with
==may not be reliable if new fields are added in the future. It's better to check if the user provided any values explicitly.Implement a method to check if
MetricsOptionshas been set:impl MetricsOptions { fn is_default(&self) -> bool { self.metrics == false && self.metrics_addr.is_unspecified() && self.metrics_port == 0 } }And use it in the merging logic:
if self.metrics.is_default() { self.metrics = config.metrics.unwrap_or_default(); }
193-193: Ohayo sensei! Remove unnecessary commented-out codeThe line
// let mut contracts = parse_erc_contracts(&args.contracts)?;seems to be leftover code. Removing it keeps the codebase clean and maintainable.Apply this diff to remove the commented code:
-// let mut contracts = parse_erc_contracts(&args.contracts)?;
304-304: Ohayo sensei! Simplify CORS origins filtering logicThe filter on
http_cors_originsmay be unnecessary if an empty vector indicates no CORS origins. Simplify the logic to improve readability.Apply this diff to simplify:
-proxy_server.set_graphql_addr(new_addr).await; +proxy_server.set_graphql_addr(new_addr).await; -let cors_origins = args.server.http_cors_origins.filter(|cors_origins| !cors_origins.is_empty()); +let cors_origins = args.server.http_cors_origins; let proxy_server = Arc::new(Proxy::new( addr, cors_origins, Some(grpc_addr), None, ));
425-432: Ohayo sensei! Add assertions forserveroptions in testsThe test
test_cli_precedencedoesn't assert theserverconfigurations. Adding these assertions ensures that command-line arguments and config file merging work as expected.Apply this diff to add assertions:
assert_eq!(torii_args.server.http_addr, IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0))); assert_eq!(torii_args.server.http_port, 8080); +assert_eq!(torii_args.server.http_cors_origins, None);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
bin/katana/src/cli/options.rs(2 hunks)bin/torii/Cargo.toml(1 hunks)bin/torii/src/main.rs(9 hunks)bin/torii/src/options.rs(1 hunks)crates/torii/core/src/types.rs(1 hunks)examples/simple/manifest_dev.json(6 hunks)
🔇 Additional comments (7)
bin/torii/Cargo.toml (1)
54-55: Ohayo sensei! The dependency changes look good!
The addition of assert_matches as a dev dependency and the presence of camino in both sections follow good Rust practices. The use of workspace inheritance for version management helps maintain consistency across the project.
examples/simple/manifest_dev.json (3)
2131-2131: Event configuration updates look good, sensei!
The class hashes for both events have been updated while maintaining their structure. The empty members array is preserved, indicating no breaking changes to event definitions.
Also applies to: 2137-2137
1246-1247: Contract deployment updates look consistent, sensei!
The contracts have been redeployed with new addresses and class hashes. The ABIs remain consistent, and init_calldata is preserved where applicable.
#!/bin/bash
# Description: Verify contract class hash consistency across configurations
echo "Checking for consistent class hash usage..."
rg "0x340e197b0fac61961591acdd872a89b0cb862893272ab72455356f5534baa7e"Also applies to: 1504-1504, 1680-1680, 1856-1857
3-4: Ohayo sensei! Verify world contract deployment consistency.
The world contract's class_hash and address have been updated. Please ensure that all dependent components and configurations are updated accordingly.
✅ Verification successful
Ohayo sensei! World contract deployment is consistent.
The new world contract address and class hash are only referenced in the manifest_dev.json file, and there are no stale references to the old values in the codebase. The deployment appears to be properly updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any references to the old world contract address
# to ensure all configurations have been updated.
echo "Searching for old world contract address..."
rg "0x30e907536f553a4b90bbd008df0785061073e432f20779e0c3549923dd67576"
echo "Searching for old world contract class hash..."
rg "0x2f92b70bd2b5a40ddef12c55257f245176870b25c7eb0bd7a60cf1f1f2fbf0e"
Length of output: 507
Script:
#!/bin/bash
# Let's search for both old and new addresses/hashes in all configuration files
echo "Searching for new world contract address..."
rg "0x1b2e50266b9b673eb82e68249a1babde860f6414737b4a36ff7b29411a64666"
echo "Searching for new world contract class hash..."
rg "0x79d9ce84b97bcc2a631996c3100d57966fc2f5b061fb1ec4dfd0040976bcac6"
# Let's also check for any manifest files to ensure consistency
echo "Listing all manifest files..."
fd "manifest.*\.json$"
# Check content of all manifest files
echo "Content of all manifest files:"
fd "manifest.*\.json$" -x cat {}
Length of output: 223265
bin/torii/src/options.rs (2)
72-73:
Ohayo, sensei! Fix the syntax error by removing the extra parenthesis.
There's an extra closing parenthesis ) after the #[arg(...)] attribute for cert_path, which causes a syntax error.
Apply this diff to fix the syntax error:
)]
- )
pub cert_path: Option<String>,Likely invalid or redundant comment.
62-63:
Ohayo, sensei! Fix the syntax error by removing the extra parenthesis.
There's an extra closing parenthesis ) after the #[arg(...)] attribute for local_key_path, which causes a syntax error.
Apply this diff to fix the syntax error:
)]
- )
pub local_key_path: Option<String>,Likely invalid or redundant comment.
bin/torii/src/main.rs (1)
Line range hint 367-373: Ohayo sensei! Handle potential broker closure gracefully
In spawn_rebuilding_graphql_server, if broker.next().await returns None, the loop breaks. Ensure this behavior is intended, and handle unexpected closures to prevent potential issues.
Consider adding error handling:
while let Some(_) = broker.next().await {
// Existing code
}
// Handle broker closure
log::warn!("Broker closed unexpectedly");| fn parse_erc_contract(part: &str) -> anyhow::Result<Contract> { | ||
| match part.split(':').collect::<Vec<&str>>().as_slice() { | ||
| [r#type, address] => { | ||
| let r#type = r#type.parse::<ContractType>()?; | ||
| if r#type == ContractType::WORLD { | ||
| return Err(anyhow::anyhow!( | ||
| "World address cannot be specified as an ERC contract" | ||
| )); | ||
| } | ||
|
|
||
| let address = Felt::from_str(address) | ||
| .with_context(|| format!("Expected address, found {}", address))?; | ||
| Ok(Contract { address, r#type }) | ||
| } | ||
| _ => Err(anyhow::anyhow!("Invalid contract format")), | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Update parse_erc_contract to handle the expected input format.
The function parse_erc_contract currently only handles inputs with two parts ([r#type, address]), but according to the comment, it should also handle inputs with three parts (erc_type:address:start_block) and defaults when only address:start_block is provided.
Would you like assistance in updating the function to correctly parse and handle all expected input formats?
| // We may add recursive merging in the future. | ||
|
|
||
| /// Enable indexing pending blocks | ||
| #[arg(long, action = ArgAction::Set, default_value_t = true)] | ||
| index_pending: bool, | ||
| if self.world_address.is_none() { | ||
| self.world_address = config.world_address; | ||
| } | ||
|
|
||
| /// Polling interval in ms | ||
| #[arg(long, default_value = "500")] | ||
| polling_interval: u64, | ||
| if self.rpc == Url::parse(DEFAULT_RPC_URL).unwrap() { | ||
| if let Some(rpc) = config.rpc { | ||
| self.rpc = rpc; | ||
| } | ||
| } | ||
|
|
||
| /// Max concurrent tasks | ||
| #[arg(long, default_value = "100")] | ||
| max_concurrent_tasks: usize, | ||
| if self.db_dir.is_none() { | ||
| self.db_dir = config.db_dir; | ||
| } | ||
|
|
||
| /// Whether or not to index world transactions | ||
| #[arg(long, action = ArgAction::Set, default_value_t = false)] | ||
| index_transactions: bool, | ||
| if self.external_url.is_none() { | ||
| self.external_url = config.external_url; | ||
| } | ||
|
|
||
| /// Whether or not to index raw events | ||
| #[arg(long, action = ArgAction::Set, default_value_t = true)] | ||
| index_raw_events: bool, | ||
| // Currently the comparison it's only at the top level. | ||
| // Need to make it more granular. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Implement recursive merging for configuration
Currently, merging is only at the top level, which may lead to incomplete configurations. Implementing recursive merging ensures all nested configurations are properly considered.
Consider using a crate like merge to recursively merge structs:
use merge::Merge;
#[derive(Debug, Serialize, Deserialize, Default, Merge)]
pub struct ToriiArgsConfig {
// ...
}
impl ToriiArgs {
pub fn with_config_file(mut self) -> Result<Self> {
// ...
let config: ToriiArgsConfig = // ...
// Merge configurations recursively
self.merge(config);
Ok(self)
}
}| if self.rpc == Url::parse(DEFAULT_RPC_URL).unwrap() { | ||
| if let Some(rpc) = config.rpc { | ||
| self.rpc = rpc; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Avoid comparing default rpc URL to determine overrides
Comparing self.rpc to DEFAULT_RPC_URL to decide whether to override it with the config file's rpc value can be fragile if the default changes. It's better to track whether the rpc was explicitly set by the user.
Consider initializing rpc as an Option<Url> and applying the default after merging with the config file:
#[derive(Parser, Debug)]
struct ToriiArgs {
- #[arg(long, value_name = "URL", default_value = DEFAULT_RPC_URL, value_parser = parse_url)]
- rpc: Url,
+ #[arg(long, value_name = "URL", value_parser = parse_url)]
+ rpc: Option<Url>,
// ...
pub fn with_config_file(mut self) -> Result<Self> {
// ...
- if self.rpc == Url::parse(DEFAULT_RPC_URL).unwrap() {
+ if self.rpc.is_none() {
if let Some(rpc) = config.rpc {
self.rpc = Some(rpc);
}
}
// After merging, apply the default if still none
+ if self.rpc.is_none() {
+ self.rpc = Some(Url::parse(DEFAULT_RPC_URL).unwrap());
+ }Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2674 +/- ##
==========================================
+ Coverage 57.40% 57.59% +0.18%
==========================================
Files 403 404 +1
Lines 50833 51065 +232
==========================================
+ Hits 29183 29409 +226
- Misses 21650 21656 +6 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests