refactor(katana): move the rpc address log #2518
Conversation
WalkthroughOhayo, sensei! This pull request introduces several modifications across multiple files in the Katana project. Key changes include the simplification of the Changes
Possibly related PRs
🪧 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: 5
🧹 Outside diff range and nitpick comments (2)
bin/katana/src/cli/node.rs (1)
237-237: Ohayo, sensei! LGTM with a small suggestion.The removal of the
addressparameter from theprint_introfunction aligns well with the PR objectives. It simplifies the function and ensures that the RPC address is not logged prematurely.However, to improve code readability, consider using the method call syntax for
print_intro:- print_intro(&self, genesis); + self.print_intro(genesis);This change would make the code more idiomatic and consistent with Rust's object-oriented style.
crates/katana/node-bindings/src/lib.rs (1)
698-708: Expand test cases forparse_rpc_addr_logfunctionOhayo sensei! The test
test_parse_rpc_addr_logis well-written. To ensure robustness, consider adding test cases for logs with missingaddr=substrings or malformed addresses to handle edge cases gracefully.
📜 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 (5)
- bin/katana/src/cli/node.rs (2 hunks)
- crates/katana/node-bindings/Cargo.toml (1 hunks)
- crates/katana/node-bindings/src/json.rs (1 hunks)
- crates/katana/node-bindings/src/lib.rs (6 hunks)
- crates/katana/node/src/lib.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
crates/katana/node-bindings/Cargo.toml (1)
12-12: Ohayo, sensei! The regex dependency looks good to me.The addition of the regex dependency aligns well with the PR objectives, particularly for parsing or validating RPC addresses. Using
.workspace = trueis a great practice for maintaining consistent versions across the project.Could you enlighten this humble reviewer about the specific use case for the regex crate in the context of RPC address logging? It would be most helpful to understand how it ties into the refactoring mentioned in the PR objectives.
✅ Verification successful
Ohayo, sensei! The addition of the regex dependency aligns perfectly with the PR objectives and ensures consistent version management across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the newly added regex dependency # Test: Search for regex usage in Rust files rg --type rust 'use\s+regex|regex::'Length of output: 627
crates/katana/node/src/lib.rs (1)
358-359: Ohayo, sensei! This log statement is a welcome addition.The new log statement effectively addresses the PR objective by relocating the RPC address log to occur precisely when the server is spawned. This change offers several benefits:
- It provides clearer feedback on the exact moment the RPC server becomes available.
- It helps users quickly identify the address they need to connect to the RPC server.
- It aligns well with the new node and configuration structures, especially in scenarios where random ports are used.
bin/katana/src/cli/node.rs (2)
Line range hint
371-467: Ohayo again, sensei! The implementation looks good.The
print_introfunction has been successfully updated to remove the address-related logging, which is consistent with the PR objectives. The function continues to log important information such as account details, seed, and genesis contracts.This change ensures that the RPC address is not logged prematurely, allowing for the correct handling of random port allocation as mentioned in the PR summary.
Line range hint
1-624: Ohayo one last time, sensei! Here's a summary of the changes.The modifications in this file successfully achieve the PR's main objective of relocating the RPC address logging. By removing the
addressparameter from theprint_introfunction and its call site, we ensure that the RPC address is not logged prematurely.These changes support the new node and configuration structures, particularly in scenarios where a random port is requested (e.g., using
--port 0). This allows thejsonrpseelibrary to handle port allocation, with the actual port number being determined after the server has been spawned.The overall impact of these changes is positive:
- It provides a clearer representation of the server's initialization point.
- It allows for more flexible port allocation, especially in random port scenarios.
- It simplifies the
print_introfunction, making it more focused on its primary responsibilities.Great work on implementing these changes, sensei! They align well with the PR objectives and improve the overall architecture of the Katana node.
crates/katana/node-bindings/src/json.rs (3)
12-16: Ohayo, sensei! Excellent use of generics inJsonLogstructBy introducing a generic type parameter
Tin theJsonLogstruct, you've enhanced flexibility for parsing different log message formats. This change allows for more scalable and adaptable code. Well done!
49-54: Ohayo, sensei! Efficient parsing withTryFrom<String>forKatanaInfoImplementing
TryFrom<String>forKatanaInfousingserde_json::from_strprovides a clean and straightforward way to parse the JSON string contained in themessagefield. This enhances readability and maintainability. Great job!
70-71: Verify the deserialization ofRpcAddr, senseiEnsure that the
addrfield in the JSON logs matches the expected format forSocketAddr. This is particularly important if the logs might contain IPv6 addresses or unusual port configurations. Verifying this will prevent potential runtime errors.Run the following script to check the format of
addrin the logs:
| pub struct Fields<T = serde_json::Value> { | ||
| pub message: String, | ||
| #[serde(flatten)] | ||
| pub other: T, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a Deserialize bound to the generic type T, sensei
Currently, the generic type T in Fields<T> does not have any trait bounds. To ensure that serde can deserialize other: T properly, it would be prudent to add a T: Deserialize<'de> constraint.
Apply this diff to add the required constraint:
#[derive(Deserialize, Debug)]
-pub struct Fields<T = serde_json::Value> {
+pub struct Fields<T = serde_json::Value>
+where
+ T: for<'de> Deserialize<'de>,
+{
pub message: String,
#[serde(flatten)]
pub other: T,
}📝 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 struct Fields<T = serde_json::Value> { | |
| pub message: String, | |
| #[serde(flatten)] | |
| pub other: T, | |
| #[derive(Deserialize, Debug)] | |
| pub struct Fields<T = serde_json::Value> | |
| where | |
| T: for<'de> Deserialize<'de>, | |
| { | |
| pub message: String, | |
| #[serde(flatten)] | |
| pub other: T, |
| dbg!(&line); | ||
|
|
There was a problem hiding this comment.
Remove dbg! macros from production code
Ohayo sensei! The dbg! macro is intended for debugging and should not be present in production code. Please remove the dbg!(&line); at line 514 and the dbg!(...) at line 519 to keep the code clean.
Apply this diff to remove the dbg! statements:
- dbg!(&line);
...
- if let Ok(log) = dbg!(serde_json::from_str::<JsonLog<RpcAddr>>(&line)) {
+ if let Ok(log) = serde_json::from_str::<JsonLog<RpcAddr>>(&line) {Also applies to: 519-519
| /// | ||
| /// This is useful for removing the color codes from the katana output. | ||
| fn clean_ansi_escape_codes(input: &str) -> Result<Cow<'_, str>, Error> { | ||
| let re = regex::Regex::new(r"\x1b\[[0-9;]*[a-zA-Z]")?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize the regex once to improve performance
Ohayo sensei! Currently, the regex in clean_ansi_escape_codes is compiled each time the function is called, which can impact performance. Consider initializing the regex once and reusing it.
Apply this diff to use Lazy from once_cell to compile the regex only once:
+use once_cell::sync::Lazy;
+use regex::Regex;
fn clean_ansi_escape_codes(input: &str) -> Result<Cow<'_, str>, Error> {
+ static ANSI_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"\x1b\[[0-9;]*[a-zA-Z]").unwrap());
- let re = regex::Regex::new(r"\x1b\[[0-9;]*[a-zA-Z]")?;
- Ok(re.replace_all(input, ""))
+ Ok(ANSI_REGEX.replace_all(input, ""))
}Remember to add once_cell = "1.18" to your Cargo.toml dependencies.
Committable suggestion was skipped due to low confidence.
| // Example RPC address log format (ansi color codes removed): | ||
| // 2024-10-10T14:20:53.563106Z INFO rpc: RPC server started. addr=127.0.0.1:60373 | ||
| fn parse_rpc_addr_log(log: &str) -> Result<SocketAddr, Error> { | ||
| // remove any ANSI escape codes from the log. | ||
| let cleaned = clean_ansi_escape_codes(log)?; | ||
|
|
||
| // This will separate the log into two parts as separated by `addr=` str and we take | ||
| // only the second part which is the address. | ||
| let addr_part = cleaned.split("addr=").nth(1).ok_or(Error::MissingSocketAddr)?; | ||
| let addr = addr_part.trim(); | ||
| Ok(SocketAddr::from_str(addr)?) | ||
| } |
There was a problem hiding this comment.
Handle potential parsing errors in parse_rpc_addr_log
Ohayo sensei! The parse_rpc_addr_log function assumes that the log contains addr=, which might not always be the case. Consider adding error handling for logs that do not contain addr= or have unexpected formats.
Apply this diff to improve error handling:
fn parse_rpc_addr_log(log: &str) -> Result<SocketAddr, Error> {
// remove any ANSI escape codes from the log.
let cleaned = clean_ansi_escape_codes(log)?;
// This will separate the log into two parts as separated by `addr=` str and we take
// only the second part which is the address.
- let addr_part = cleaned.split("addr=").nth(1).ok_or(Error::MissingSocketAddr)?;
+ let addr_part = cleaned.split("addr=").nth(1)
+ .ok_or_else(|| Error::UnexpectedFormat(format!("Missing 'addr=' in log: {}", cleaned)))?;
let addr = addr_part.trim();
Ok(SocketAddr::from_str(addr)?)
}This ensures that any unexpected formats are reported clearly.
📝 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.
| // Example RPC address log format (ansi color codes removed): | |
| // 2024-10-10T14:20:53.563106Z INFO rpc: RPC server started. addr=127.0.0.1:60373 | |
| fn parse_rpc_addr_log(log: &str) -> Result<SocketAddr, Error> { | |
| // remove any ANSI escape codes from the log. | |
| let cleaned = clean_ansi_escape_codes(log)?; | |
| // This will separate the log into two parts as separated by `addr=` str and we take | |
| // only the second part which is the address. | |
| let addr_part = cleaned.split("addr=").nth(1).ok_or(Error::MissingSocketAddr)?; | |
| let addr = addr_part.trim(); | |
| Ok(SocketAddr::from_str(addr)?) | |
| } | |
| fn parse_rpc_addr_log(log: &str) -> Result<SocketAddr, Error> { | |
| // remove any ANSI escape codes from the log. | |
| let cleaned = clean_ansi_escape_codes(log)?; | |
| // This will separate the log into two parts as separated by `addr=` str and we take | |
| // only the second part which is the address. | |
| let addr_part = cleaned.split("addr=").nth(1) | |
| .ok_or_else(|| Error::UnexpectedFormat(format!("Missing 'addr=' in log: {}", cleaned)))?; | |
| let addr = addr_part.trim(); | |
| Ok(SocketAddr::from_str(addr)?) | |
| } |
| /// Removes ANSI escape codes from a string. | ||
| /// | ||
| /// This is useful for removing the color codes from the katana output. | ||
| fn clean_ansi_escape_codes(input: &str) -> Result<Cow<'_, str>, Error> { | ||
| let re = regex::Regex::new(r"\x1b\[[0-9;]*[a-zA-Z]")?; | ||
| Ok(re.replace_all(input, "")) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a dedicated crate to handle ANSI escape codes
Ohayo sensei! Instead of manually handling ANSI escape codes, consider using a dedicated crate like ansi_strip or strip-ansi-escapes for more robust handling.
Apply this diff to use the strip-ansi-escapes crate:
+use strip_ansi_escapes::strip.as_bytes;
fn clean_ansi_escape_codes(input: &str) -> Result<Cow<'_, str>, Error> {
- let re = regex::Regex::new(r"\x1b\[[0-9;]*[a-zA-Z]")?;
- Ok(re.replace_all(input, ""))
+ let stripped = strip_ansi_escapes::strip(input.as_bytes())?;
+ Ok(Cow::Owned(String::from_utf8_lossy(&stripped).into_owned()))
}Don't forget to add strip-ansi-escapes = "0.1.0" to your Cargo.toml.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/ci.yml (1)
43-49: Ohayo, sensei! These changes look great!The modifications to the
testjob streamline the testing process and align well with the PR objectives. Here's a breakdown of the improvements:
- Adding
/tmp/binstoPATHallows direct access tokatanaandsozobinaries.- Setting execute permissions ensures the binaries can be run.
- Extracting tar files to
/tmp/provides necessary test data.- Simplifying the
cargo llvm-cov nextestcommand suggests a more straightforward test execution.These changes should make the CI process more efficient and easier to maintain.
Consider adding a comment explaining the purpose of the tar file extractions for better clarity:
+ # Extract test databases tar -xzf spawn-and-move-db.tar.gz -C /tmp/ tar -xzf types-test-db.tar.gz -C /tmp/
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2518 +/- ##
=======================================
Coverage 68.55% 68.55%
=======================================
Files 387 387
Lines 49950 49968 +18
=======================================
+ Hits 34243 34258 +15
- Misses 15707 15710 +3 ☔ View full report in Codecov by Sentry. |
This reverts commit 58a2a34.
ref #2514
moving this exactly when the server is spawned, to better reflect the server initialization point. also because with the new node and config structures, when user request for a random port (ie
--port 0) because we delegate the search for a free port to thejsonrpsee(ie we just pass the raw ip address with port 0 when we build the rpc server), we can only know the actual port number once the server has been spawned. we could technically find the free port first on the node level so that we can determined the full address beforehand. but i prefer this approach.Summary by CodeRabbit
New Features
Bug Fixes
Chores
regexfor enhanced functionality.