feat(torii): erc options for max tasks & artifacts path #3061
Conversation
WalkthroughOhayo, sensei! This PR refines the command-line interface for Torii by removing the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 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
🧹 Nitpick comments (4)
crates/torii/runner/src/lib.rs (1)
145-145: Ohayo sensei! Consider using ERC-specific default for max concurrent tasks.The code is using
DEFAULT_MAX_CONCURRENT_TASKS(100) for both indexing and ERC tasks. Since ERC token indexing might have different resource requirements, consider using the ERC-specific defaultDEFAULT_ERC_MAX_CONCURRENT_TASKS(10) defined inoptions.rs.crates/torii/cli/src/options.rs (3)
355-356: Ohayo! Use ERC-specific default and improve help text.Two suggestions for improvement:
- Use
DEFAULT_ERC_MAX_CONCURRENT_TASKSinstead ofDEFAULT_MAX_CONCURRENT_TASKSfor consistency.- Add more descriptive help text about the impact of concurrent tasks on resource usage.
- #[arg(long = "erc.max_concurrent_tasks", default_value_t = DEFAULT_MAX_CONCURRENT_TASKS)] + #[arg( + long = "erc.max_concurrent_tasks", + default_value_t = DEFAULT_ERC_MAX_CONCURRENT_TASKS, + help = "Maximum number of concurrent tasks for indexing ERC tokens. Higher values increase memory usage." + )] pub max_concurrent_tasks: usize,
359-360: Add help text for artifacts path.Consider adding descriptive help text for the artifacts path option.
- #[arg(long)] + #[arg( + long = "erc.artifacts_path", + help = "Directory path to store ERC token metadata and media files. Defaults to a temporary directory." + )] pub artifacts_path: Option<Utf8PathBuf>,
364-366: Use ERC-specific default in Default implementation.For consistency, use
DEFAULT_ERC_MAX_CONCURRENT_TASKSin the Default implementation.fn default() -> Self { - Self { max_concurrent_tasks: DEFAULT_MAX_CONCURRENT_TASKS, artifacts_path: None } + Self { max_concurrent_tasks: DEFAULT_ERC_MAX_CONCURRENT_TASKS, artifacts_path: None } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/cli/src/args.rs(4 hunks)crates/torii/cli/src/options.rs(3 hunks)crates/torii/runner/src/lib.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (6)
crates/torii/cli/src/args.rs (4)
51-52: Ohayo! Nice addition of ERC options!The flattening of ERC options into the command-line arguments is well-implemented and follows Rust's command-line argument parsing best practices.
106-108: LGTM! Clean configuration merging logic.The configuration merging logic for ERC options follows the same pattern as other options, maintaining consistency.
138-138: LGTM! Consistent configuration structure.The addition of ERC options to the configuration structure is clean and consistent with other option types.
169-169: LGTM! Clean serialization logic.The serialization logic for ERC options follows the same pattern as other options, maintaining consistency.
crates/torii/runner/src/lib.rs (1)
209-213: LGTM! Clean artifacts path handling.The artifacts path handling with fallback to a temporary directory is well-implemented.
crates/torii/cli/src/options.rs (1)
23-23: LGTM! Good default value for ERC tasks.The default value of 10 for ERC concurrent tasks is reasonable and helps prevent resource exhaustion.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/cli/src/options.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: build
🔇 Additional comments (3)
crates/torii/cli/src/options.rs (3)
5-5: Ohayo! The new imports and constants look good, sensei!The
DEFAULT_ERC_MAX_CONCURRENT_TASKSconstant with a value of 10 is a reasonable default for managing ERC token metadata indexing tasks.Also applies to: 23-24
123-124: Documentation improvement looks good!The updated help text for
max_concurrent_tasksinIndexingOptionsis now more precise and clearer about its purpose.Also applies to: 127-127
463-465: Helper function looks good!The
default_erc_max_concurrent_tasksfunction correctly returns the ERC-specific default value.
crates/torii/cli/src/options.rs
Outdated
| #[derive(Debug, clap::Args, Clone, Serialize, Deserialize, PartialEq)] | ||
| #[command(next_help_heading = "ERC options")] | ||
| pub struct ErcOptions { | ||
| /// The maximum number of concurrent tasks to use for indexing ERC721 and ERC1155 token | ||
| /// metadata. | ||
| #[arg( | ||
| long = "erc.max_concurrent_tasks", | ||
| default_value_t = DEFAULT_ERC_MAX_CONCURRENT_TASKS, | ||
| help = "The maximum number of concurrent tasks to use for indexing ERC721 and ERC1155 token metadata." | ||
| )] | ||
| #[serde(default = "default_erc_max_concurrent_tasks")] | ||
| pub max_concurrent_tasks: usize, | ||
|
|
||
| /// Path to a directory to store ERC artifacts | ||
| #[arg(long)] | ||
| pub artifacts_path: Option<Utf8PathBuf>, | ||
| } | ||
|
|
||
| impl Default for ErcOptions { | ||
| fn default() -> Self { | ||
| Self { max_concurrent_tasks: DEFAULT_MAX_CONCURRENT_TASKS, artifacts_path: None } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo! The new ErcOptions structure looks well-designed, sensei!
The structure provides a clean separation of concerns for ERC-specific configuration. However, there's a small issue in the default implementation.
The default implementation uses DEFAULT_MAX_CONCURRENT_TASKS (100) instead of DEFAULT_ERC_MAX_CONCURRENT_TASKS (10). Apply this diff to fix:
- Self { max_concurrent_tasks: DEFAULT_MAX_CONCURRENT_TASKS, artifacts_path: None }
+ Self { max_concurrent_tasks: DEFAULT_ERC_MAX_CONCURRENT_TASKS, artifacts_path: None }📝 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.
| #[derive(Debug, clap::Args, Clone, Serialize, Deserialize, PartialEq)] | |
| #[command(next_help_heading = "ERC options")] | |
| pub struct ErcOptions { | |
| /// The maximum number of concurrent tasks to use for indexing ERC721 and ERC1155 token | |
| /// metadata. | |
| #[arg( | |
| long = "erc.max_concurrent_tasks", | |
| default_value_t = DEFAULT_ERC_MAX_CONCURRENT_TASKS, | |
| help = "The maximum number of concurrent tasks to use for indexing ERC721 and ERC1155 token metadata." | |
| )] | |
| #[serde(default = "default_erc_max_concurrent_tasks")] | |
| pub max_concurrent_tasks: usize, | |
| /// Path to a directory to store ERC artifacts | |
| #[arg(long)] | |
| pub artifacts_path: Option<Utf8PathBuf>, | |
| } | |
| impl Default for ErcOptions { | |
| fn default() -> Self { | |
| Self { max_concurrent_tasks: DEFAULT_MAX_CONCURRENT_TASKS, artifacts_path: None } | |
| } | |
| } | |
| #[derive(Debug, clap::Args, Clone, Serialize, Deserialize, PartialEq)] | |
| #[command(next_help_heading = "ERC options")] | |
| pub struct ErcOptions { | |
| /// The maximum number of concurrent tasks to use for indexing ERC721 and ERC1155 token | |
| /// metadata. | |
| #[arg( | |
| long = "erc.max_concurrent_tasks", | |
| default_value_t = DEFAULT_ERC_MAX_CONCURRENT_TASKS, | |
| help = "The maximum number of concurrent tasks to use for indexing ERC721 and ERC1155 token metadata." | |
| )] | |
| #[serde(default = "default_erc_max_concurrent_tasks")] | |
| pub max_concurrent_tasks: usize, | |
| /// Path to a directory to store ERC artifacts | |
| #[arg(long)] | |
| pub artifacts_path: Option<Utf8PathBuf>, | |
| } | |
| impl Default for ErcOptions { | |
| fn default() -> Self { | |
| Self { max_concurrent_tasks: DEFAULT_ERC_MAX_CONCURRENT_TASKS, artifacts_path: None } | |
| } | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3061 +/- ##
==========================================
+ Coverage 57.49% 57.50% +0.01%
==========================================
Files 439 439
Lines 59820 59833 +13
==========================================
+ Hits 34392 34406 +14
+ Misses 25428 25427 -1 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Refactor