feat(katana): exact path for chain config #3009
Conversation
kariy
force-pushed
the
katana/init-output-path
branch
from
8a5437c to
9504dc5
Compare
WalkthroughOhayo sensei! This PR reworks the chain configuration management system across the CLI commands and chain-spec modules. In the config command, it replaces the use of Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant I as Init Command (execute)
participant D as Directory Manager
U->>I: Invoke init command with (optional) output_path and chain_spec
alt output_path provided
I->>D: Create ChainConfigDir using the provided output_path
else No output_path
I->>D: Open default LocalChainConfigDir
end
D-->>I: Return configuration directory path
I->>I: Write chain_spec to the designated directory
sequenceDiagram
participant U as User
participant C as Config Command (execute)
participant D as Directory Manager
participant S as ChainSpec Source
U->>C: Invoke config command with a chain parameter
C->>D: Open configuration directory using LocalChainConfigDir
D-->>C: Return configuration path
C->>S: Retrieve chain listing via updated method (`katana_chain_spec::rollup::list`)
S-->>C: Provide chain configuration data
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: 0
🧹 Nitpick comments (8)
crates/katana/chain-spec/src/rollup/file.rs (2)
43-45: Ohayo sensei! Thewrite_localfunction is symmetrical toread_localand focused on local directories. Consider using an atomic file-writing approach if partial writes are a concern, but overall this is good.
113-113: Ohayo sensei! CallingLocalChainConfigDir::open_at(...).expect("must exist")is convenient in tests, but handling the error might be safer if unexpected conditions occur.crates/katana/chain-spec/src/rollup/mod.rs (1)
12-12: Consider using explicit re-exports instead of wildcards.While
pub use file::*;maintains backward compatibility, explicit re-exports likepub use file::{ChainConfigDir, read, write};make the public API more obvious and maintainable.crates/katana/cli/src/utils.rs (1)
241-259: Ohayo sensei! Nice handling of path/chain ID ambiguity!The function elegantly handles both path and chain ID inputs, with clear error messages and path expansion support.
Consider adding more robust path validation.
The current implementation could benefit from additional path validation:
- Check for directory existence before opening
- Validate path permissions
- Handle symlinks securely
pub fn parse_chain_config_dir(value: &str) -> Result<ChainConfigDir> { let path = PathBuf::from(value); if path.components().count() == 1 { if path.exists() { + // Check if it's a directory and has correct permissions + if !path.is_dir() { + return Err(anyhow!("Path exists but is not a directory")); + } + if !path.metadata()?.permissions().readonly() { + return Err(anyhow!("Directory requires read permissions")); + } Ok(ChainConfigDir::open(path)?) } else if let Ok(id) = ChainId::parse(value) { Ok(ChainConfigDir::open_local(&id)?) } else { Err(anyhow!("Invalid path or chain id")) } } else { let path = PathBuf::from(shellexpand::tilde(value).as_ref()); + // Same validation for expanded paths + if path.exists() && !path.is_dir() { + return Err(anyhow!("Path exists but is not a directory")); + } Ok(ChainConfigDir::open(path)?) } }bin/katana/src/cli/init/mod.rs (2)
48-50: Enhance field documentation with path format details.Consider adding more details to the documentation:
- Expected path format (absolute/relative)
- Path requirements (permissions, existing vs. new directory)
- Examples of valid paths
- /// Specify the path of the directory where the configuration files will be stored at. + /// Specify the path of the directory where the configuration files will be stored. + /// + /// The path can be: + /// - Absolute path: `/path/to/config` + /// - Relative path: `./config` or `config` + /// - Home directory: `~/config` + /// + /// The directory will be created if it doesn't exist. Write permissions are required. + /// + /// Example: `--output-path ~/katana/chains/testnet`
76-83: Add user feedback about config file location.Consider informing the user where the config was written for better UX:
if let Some(path) = self.output_path { let dir = ChainConfigDir::create(path)?; rollup::write(&dir, &chain_spec).context("failed to write chain spec file")?; + println!("Chain configuration written to: {}", dir.display()); } else { // Write to the local chain config directory by default if user // doesn't specify the output path rollup::write_local(&chain_spec).context("failed to write chain spec file")?; + println!("Chain configuration written to local config directory"); }crates/katana/cli/src/args.rs (2)
47-49: Update field documentation to reflect new behavior.The documentation should be updated to explain that the field now accepts both paths and chain IDs:
- /// Path to the chain configuration file. + /// Path to the chain configuration directory or a chain ID. + /// + /// This can be: + /// - A path to a directory containing chain configuration + /// - A chain ID that maps to a local configuration + /// + /// Examples: + /// - Path: `~/katana/chains/testnet` + /// - Chain ID: `SN_GOERLI`
246-248: Document sequencer address override behavior.Add a comment explaining why the sequencer address is being overridden:
if let Some(path) = &self.chain { let mut cs = katana_chain_spec::rollup::read(path)?; + // Override sequencer address to ensure consistent behavior across all chain configurations cs.genesis.sequencer_address = *DEFAULT_SEQUENCER_ADDRESS; Ok(Arc::new(ChainSpec::Rollup(cs))) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bin/katana/src/cli/config.rs(2 hunks)bin/katana/src/cli/init/mod.rs(3 hunks)bin/katana/src/cli/mod.rs(1 hunks)crates/katana/chain-spec/src/rollup/file.rs(15 hunks)crates/katana/chain-spec/src/rollup/mod.rs(1 hunks)crates/katana/cli/src/args.rs(4 hunks)crates/katana/cli/src/utils.rs(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/katana/cli/src/utils.rs
[warning] 239-239: Line exceeds the recommended length. Consider breaking the line for better readability.
🔇 Additional comments (21)
crates/katana/chain-spec/src/rollup/file.rs (16)
1-1: Ohayo sensei! This import adjustment to includefs::selfis properly placed and consistent with the file's functionality.
19-20: Ohayo sensei! IntroducingLocalConfigDirectoryNotFoundis a clear and helpful way to provide more contextual error messages.
22-23: Ohayo sensei! AddingMustBeADirectorysimilarly improves error clarity for incorrect filesystem usage.
38-40: Ohayo sensei! The newread_localfunction is well-named and aligns with local configuration retrieval. Nicely done.
56-56: Ohayo sensei! Changing this signature to acceptChainConfigDiris consistent with the new directory-based management.
268-268: Ohayo sensei! Importingstd::fsfor these test scenarios is straightforward and purposeful.
279-279: Ohayo sensei! This import expansion to includeChainConfigDirand others aligns the tests with the new file-based approach.
291-294: Ohayo sensei! The test logic for reading the chain spec with a local dir is coherent and helps ensure correct behavior.
300-303: Ohayo sensei! The parallel approach for writing in tests nicely matches the read tests.
305-305: Ohayo sensei! Providing additionalimplforLocalChainConfigDirin tests is a clean way to manage temporary directories.
347-347: Ohayo sensei! Creating a temporary local chain config directory in this test is consistent with your new pattern.
351-351: Ohayo sensei! Opening the same directory ensures coverage for both creation and retrieval flows.
356-359: Ohayo sensei! Validating that an error is thrown for a nonexistent directory is an excellent negative test case.
371-371: Ohayo sensei! Using the temporary creation again for synergy with other chain ID tests is consistent.
395-398: Ohayo sensei! This snippet elegantly loops and writes multiple chain specs for thorough test coverage.
404-426: Ohayo sensei! The new test suite for absolute directory handling is robust, verifying creation, opening, and error states. Nicely done.bin/katana/src/cli/config.rs (3)
3-3: Ohayo sensei! ImportingLocalChainConfigDirclarifies that this config approach is focused on local directories.
18-18: Ohayo sensei! Nicely switching toLocalChainConfigDir::open(&chain)?.config_path()ensures a more direct local config handling.
24-24: Ohayo sensei! Moving tokatana_chain_spec::rollup::list()centralizes the chain listing logic, keeping the CLI thinner.bin/katana/src/cli/mod.rs (1)
42-42: Ohayo sensei! BoxingInitArgscan help manage memory usage or future growth of that struct.crates/katana/chain-spec/src/rollup/mod.rs (1)
9-10: Ohayo sensei! Good encapsulation practice!Making the
filemodule private helps hide implementation details while keeping the interface clean.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/katana/cli/src/utils.rs (1)
244-259: Consider adding path validation and error context.While the implementation is solid, a few improvements could enhance robustness and user experience:
- Add path validation to ensure no directory traversal
- Provide more context in error messages
pub fn parse_chain_config_dir(value: &str) -> Result<ChainConfigDir> { let path = PathBuf::from(value); + // Ensure path is canonical and doesn't contain directory traversal + if path.components().any(|c| matches!(c, std::path::Component::ParentDir)) { + return Err(anyhow!("Path must not contain directory traversal (..)")); + } if path.components().count() == 1 { if path.exists() { Ok(ChainConfigDir::open(path)?) } else if let Ok(id) = ChainId::parse(value) { Ok(ChainConfigDir::open_local(&id)?) } else { - Err(anyhow!("Invalid path or chain id")) + Err(anyhow!("'{}' is neither a valid path nor a valid chain ID", value)) } } else { let path = PathBuf::from(shellexpand::tilde(value).as_ref()); Ok(ChainConfigDir::open(path)?) } }bin/katana/src/cli/init/mod.rs (1)
76-83: Consider adding path existence check and user feedback.The conditional logic for handling output paths could benefit from additional user feedback and validation.
if let Some(path) = self.output_path { + if path.exists() && path.read_dir()?.next().is_some() { + return Err(anyhow!("Output directory '{}' is not empty", path.display())); + } let dir = ChainConfigDir::create(path)?; rollup::write(&dir, &chain_spec).context("failed to write chain spec file")?; + info!(target: LOG_TARGET, "Chain configuration written to {}", dir.config_path().display()); } else { // Write to the local chain config directory by default if user // doesn't specify the output path rollup::write_local(&chain_spec).context("failed to write chain spec file")?; + info!(target: LOG_TARGET, "Chain configuration written to local directory"); }crates/katana/cli/src/args.rs (1)
245-250: Consider adding validation for chain configuration.The chain_spec method could benefit from additional validation of the configuration.
if let Some(path) = &self.chain { let mut cs = katana_chain_spec::rollup::read(path)?; + // Validate essential configuration parameters + if cs.genesis.sequencer_address == ContractAddress::default() { + return Err(anyhow::anyhow!("Invalid sequencer address in chain configuration")); + } cs.genesis.sequencer_address = *DEFAULT_SEQUENCER_ADDRESS; Ok(Arc::new(ChainSpec::Rollup(cs))) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bin/katana/src/cli/config.rs(2 hunks)bin/katana/src/cli/init/mod.rs(3 hunks)bin/katana/src/cli/mod.rs(1 hunks)crates/katana/chain-spec/src/rollup/file.rs(15 hunks)crates/katana/chain-spec/src/rollup/mod.rs(1 hunks)crates/katana/cli/src/args.rs(4 hunks)crates/katana/cli/src/utils.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- bin/katana/src/cli/mod.rs
- crates/katana/chain-spec/src/rollup/mod.rs
- bin/katana/src/cli/config.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/katana/cli/src/utils.rs
[warning] 239-239: Line exceeds the recommended length. Consider breaking the line for better readability.
🔇 Additional comments (4)
crates/katana/cli/src/utils.rs (1)
241-244: LGTM! Clear documentation of the parsing strategy.The comments effectively explain the ambiguity between chain IDs and paths, and how it's resolved.
bin/katana/src/cli/init/mod.rs (1)
48-50: LGTM! Well-documented new parameter.The documentation clearly explains the purpose of the
output_pathparameter.crates/katana/chain-spec/src/rollup/file.rs (1)
138-141: LGTM! Clean enum design for configuration paths.The
ChainConfigDirenum provides a clear separation between local and absolute paths.crates/katana/cli/src/args.rs (1)
46-49: LGTM! Clear documentation and proper value parser.The chain argument is well-documented and uses the new parsing function.
| pub fn create<P: AsRef<Path>>(path: P) -> Result<Self, Error> { | ||
| let path = path.as_ref(); | ||
|
|
||
| if !path.exists() { | ||
| std::fs::create_dir_all(path)?; | ||
| } | ||
|
|
||
| Ok(ChainConfigDir::Absolute(path.to_path_buf())) | ||
| } | ||
|
|
||
| pub fn open<P: AsRef<Path>>(path: P) -> Result<Self, Error> { | ||
| let path = fs::canonicalize(path)?; | ||
|
|
||
| if !path.is_dir() { | ||
| return Err(Error::MustBeADirectory); | ||
| } | ||
|
|
||
| Ok(Self::Absolute(path.to_path_buf())) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding path normalization and security checks.
The path handling in create and open methods could be enhanced with additional security measures.
pub fn create<P: AsRef<Path>>(path: P) -> Result<Self, Error> {
let path = path.as_ref();
+ // Normalize path and check for directory traversal
+ let path = path.canonicalize().unwrap_or_else(|_| path.to_path_buf());
+ if path.components().any(|c| matches!(c, std::path::Component::ParentDir)) {
+ return Err(Error::MustBeADirectory);
+ }
if !path.exists() {
std::fs::create_dir_all(path)?;
}
Ok(ChainConfigDir::Absolute(path.to_path_buf()))
}📝 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.
| pub fn create<P: AsRef<Path>>(path: P) -> Result<Self, Error> { | |
| let path = path.as_ref(); | |
| if !path.exists() { | |
| std::fs::create_dir_all(path)?; | |
| } | |
| Ok(ChainConfigDir::Absolute(path.to_path_buf())) | |
| } | |
| pub fn open<P: AsRef<Path>>(path: P) -> Result<Self, Error> { | |
| let path = fs::canonicalize(path)?; | |
| if !path.is_dir() { | |
| return Err(Error::MustBeADirectory); | |
| } | |
| Ok(Self::Absolute(path.to_path_buf())) | |
| } | |
| pub fn create<P: AsRef<Path>>(path: P) -> Result<Self, Error> { | |
| let path = path.as_ref(); | |
| // Normalize path and check for directory traversal | |
| let path = path.canonicalize().unwrap_or_else(|_| path.to_path_buf()); | |
| if path.components().any(|c| matches!(c, std::path::Component::ParentDir)) { | |
| return Err(Error::MustBeADirectory); | |
| } | |
| if !path.exists() { | |
| std::fs::create_dir_all(path)?; | |
| } | |
| Ok(ChainConfigDir::Absolute(path.to_path_buf())) | |
| } | |
| pub fn open<P: AsRef<Path>>(path: P) -> Result<Self, Error> { | |
| let path = fs::canonicalize(path)?; | |
| if !path.is_dir() { | |
| return Err(Error::MustBeADirectory); | |
| } | |
| Ok(Self::Absolute(path.to_path_buf())) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/katana/cli/src/utils.rs (1)
244-259: Ohayo sensei! The implementation looks solid, but could use some enhancements.The function effectively handles both path-based and chain ID-based inputs. However, consider these improvements:
- The error message could be more descriptive by including the actual value that failed parsing.
- The function would benefit from unit tests to verify the behavior with various input types.
Here's a suggested improvement for the error message:
- Err(anyhow!("Invalid path or chain id")) + Err(anyhow!("Invalid path or chain id: {}", value))Would you like me to help generate unit tests for this function? We should test cases like:
- Single-component existing path
- Single-component non-existing path that's a valid chain ID
- Single-component invalid input
- Multi-component path with tilde expansion
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/katana/cli/src/utils.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: docs
- GitHub Check: ensure-wasm
🔇 Additional comments (2)
crates/katana/cli/src/utils.rs (2)
4-4: Ohayo sensei! The imports look good!The new imports provide the necessary types and error handling capabilities for the chain configuration directory parsing functionality.
Also applies to: 8-8, 11-11
241-244: Clear and helpful documentation!The comments effectively explain the ambiguity between chain IDs and filesystem paths, and how the function handles this ambiguity.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3009 +/- ##
==========================================
+ Coverage 56.41% 56.43% +0.01%
==========================================
Files 431 431
Lines 57162 57240 +78
==========================================
+ Hits 32250 32302 +52
- Misses 24912 24938 +26 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you @kariy for that, works like a charm.
The only comment would be that if the directory provided to read the chain exists, but doesn't contain the configuration files, the error is a bit cryptic:
cargo run --bin katana -r -- --chain /tmp/empty_folder
kariy
force-pushed
the
katana/init-output-path
branch
from
1dbf887 to
261ff47
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/katana/cli/src/utils.rs (1)
241-259: 🛠️ Refactor suggestionOhayo sensei! Consider adding path normalization and security checks.
The path handling could be enhanced with additional security measures to prevent directory traversal attacks.
Apply this diff to add security checks:
pub fn parse_chain_config_dir(value: &str) -> Result<ChainConfigDir> { let path = PathBuf::from(value); + // Normalize path and check for directory traversal + let path = path.canonicalize().unwrap_or_else(|_| path.to_path_buf()); + if path.components().any(|c| matches!(c, std::path::Component::ParentDir)) { + return Err(anyhow!("Invalid path: directory traversal not allowed")); + } if path.components().count() == 1 { if path.exists() { Ok(ChainConfigDir::open(path)?) } else if let Ok(id) = ChainId::parse(value) { Ok(ChainConfigDir::open_local(&id)?) } else { Err(anyhow!("Invalid path or chain id")) } } else { let path = PathBuf::from(shellexpand::tilde(value).as_ref()); + // Apply same security checks for expanded path + let path = path.canonicalize().unwrap_or_else(|_| path.to_path_buf()); + if path.components().any(|c| matches!(c, std::path::Component::ParentDir)) { + return Err(anyhow!("Invalid path: directory traversal not allowed")); + } Ok(ChainConfigDir::open(path)?) } }
🧹 Nitpick comments (1)
crates/katana/cli/src/args.rs (1)
46-49: Update help text to reflect path support.The current help text doesn't mention that the chain argument now accepts both paths and chain IDs.
Apply this diff to improve the help text:
- /// Path to the chain configuration file. + /// Path to the chain configuration directory or a chain ID. + /// Can be either a filesystem path or a chain identifier. #[arg(long, hide = true)] #[arg(value_parser = parse_chain_config_dir)] pub chain: Option<ChainConfigDir>,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bin/katana/src/cli/config.rs(2 hunks)bin/katana/src/cli/init/mod.rs(3 hunks)bin/katana/src/cli/mod.rs(1 hunks)crates/katana/chain-spec/src/rollup/file.rs(14 hunks)crates/katana/chain-spec/src/rollup/mod.rs(1 hunks)crates/katana/cli/src/args.rs(4 hunks)crates/katana/cli/src/utils.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- bin/katana/src/cli/mod.rs
- bin/katana/src/cli/init/mod.rs
- crates/katana/chain-spec/src/rollup/mod.rs
- bin/katana/src/cli/config.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: clippy
- GitHub Check: docs
🔇 Additional comments (3)
crates/katana/chain-spec/src/rollup/file.rs (2)
179-187: Ohayo! Good implementation of path validation.The function properly normalizes paths and validates that the target is a directory.
14-42: Nice error handling implementation!The error handling is comprehensive with clear messages and proper use of thiserror.
crates/katana/cli/src/args.rs (1)
245-280: Well-structured chain configuration handling!The changes properly support both path-based and development configurations while maintaining backward compatibility.
Allow specifying the exact path at which the chain config dir will be created on
katana init. The path can then be used as the value tokatana --chain <PATH_OR_ID>.Summary by CodeRabbit