refactor(torii): cli args & rpc options #3094
Conversation
|
Ohayo, sensei! WalkthroughThis pull request introduces significant changes to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (9)
🪧 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: 0
🔭 Outside diff range comments (1)
crates/torii/cli/src/args.rs (1)
175-326: 💡 Verification agent🧩 Analysis chain
Tests might need updating for new RPC options structure
Ohayo, sensei! I noticed that the test cases still contain direct assertions against
rpcas a URL (like line 228). These tests might need to be updated to work with the newRpcOptionsstructure instead.
🏁 Script executed:
#!/bin/bash # Check if the tests in args.rs still compile after the changes cd crates/torii/cli cargo test --lib args::test::test_cli_precedence -- --nocapture || echo "Test might need updating to match the new RpcOptions structure"Length of output: 318
Ohayo, sensei! Please update the tests for the new RPC options structure.
It appears that in
crates/torii/cli/src/args.rs(particularly in thetest_cli_precedencetest), the assertions are still directly comparing therpcfield as a URL. Since the latest changes introduced the newRpcOptionsstructure, these tests need to be updated accordingly. For example:
- Instead of asserting with
Url::parse("http://0.0.0.0:6060"), the test should now verify the corresponding value within the newRpcOptionsformat.Let's update the tests so that they reflect the structure and behavior of
RpcOptions.
🧹 Nitpick comments (1)
crates/torii/cli/src/options.rs (1)
132-133: Consider formatting the default implementation for better readabilityOhayo, sensei! The current single-line implementation of
DefaultforRpcOptionsis a bit cramped. For consistency with otherDefaultimplementations in this file (likeRelayOptions,IndexingOptions, etc.), consider using a multi-line format:- fn default() -> Self { - Self { strict_model_reader: false, events_chunk_size: DEFAULT_EVENTS_CHUNK_SIZE, url: Url::parse(DEFAULT_RPC_URL).unwrap() } + fn default() -> Self { + Self { + strict_model_reader: false, + events_chunk_size: DEFAULT_EVENTS_CHUNK_SIZE, + url: Url::parse(DEFAULT_RPC_URL).unwrap(), + } + }This makes the code more readable and consistent with the rest of the codebase.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/torii/cli/src/args.rs(4 hunks)crates/torii/cli/src/options.rs(4 hunks)crates/torii/cli/src/args.rs(0 hunks)crates/torii/cli/src/options.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: ensure-wasm
- GitHub Check: docs
🔇 Additional comments (4)
crates/torii/cli/src/args.rs (4)
41-44: LGTM: Good refactoring of RPC optionsOhayo! The extraction of RPC-related options into a dedicated struct looks clean and maintainable, sensei. This change improves code organization by grouping related configuration options together.
95-97: LGTM: Consistent handling of RPC options in config fileThe implementation correctly handles merging RPC options from the config file. Good job, sensei!
127-127: LGTM: Config structure updated properlyThe
ToriiArgsConfigstruct has been correctly updated to use the newRpcOptionstype.
157-157: LGTM: Consistent implementation in TryFromThe conversion logic properly handles the new RPC options structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/torii/cli/src/options.rs (3)
109-128: Ohayo sensei, consider improving the help text forstrict_model_reader.
Although the field is self-explanatory, you might want to give a direct usage example for clarity.For example:
- help = "Whether or not to read models from the block number they were registered in." + help = "If true, models start reading at their registration block. If false, models are read from the latest block."
130-134: Ohayo sensei, handle the parse error gracefully.
Currently, calling.unwrap()onUrl::parse(DEFAULT_RPC_URL)risks a panic if the constant ever changes or is malformed.- url: Url::parse(DEFAULT_RPC_URL).unwrap() + url: Url::parse(DEFAULT_RPC_URL).expect("Invalid default RPC URL")
417-426: Ohayo sensei, the naming might be confusing fordb_dir.
The doc suggests a file path, but the field name implies a directory. Consider renaming if it genuinely references a file.- pub db_dir: Option<PathBuf>, + pub db_file: Option<PathBuf>,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/cli/src/options.rs(4 hunks)crates/torii/cli/src/options.rs(1 hunks)crates/torii/cli/src/options.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/cli/src/options.rs
- crates/torii/cli/src/options.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: clippy
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (5)
crates/torii/cli/src/options.rs (5)
2-2: Ohayo sensei, thanks for addingPathBuf!
No issues found with this addition.
7-7: Ohayo sensei, nice import ofparse_url.
This will come in handy for handling RPC endpoints.
12-12: Ohayo sensei, good call adding theurlcrate.
This aligns well with robust URL parsing needs.
25-25: Ohayo sensei, default RPC URL looks fine.
Specifying the default athttp://0.0.0.0:5050is consistent with local development usage.
136-138: Ohayo sensei, all is well for this reconfirmedIndexingOptions.
No conflicts spotted after removingstrict_model_readerfrom here.
| /// Database filepath (ex: indexer.db). If specified file doesn't exist, it will be | ||
| /// created. Defaults to in-memory database. | ||
| #[arg( | ||
| long = "sql.db_dir", |
There was a problem hiding this comment.
The is a breaking change that could also affect slot and we would need conditional code depending on versions again in slot.
Wdyt about keeping the db-dir as it was at the moment?
There was a problem hiding this comment.
Or we could add an alias to ensure it's backward compatible.
There was a problem hiding this comment.
example of alias @Larkooo:
dojo/crates/katana/cli/src/options.rs
Line 134 in 649cc54
Summary by CodeRabbit
RpcOptionsandSqlOptionsfor improved clarity and consistency in naming conventions.