Improve Katana runner macro #2464
Conversation
WalkthroughOhayo, sensei! This pull request brings substantial updates to the project's structure and testing framework. Key changes include modifications to the Changes
Possibly related PRs
Suggested labels
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
Tip Early access features: enabledWe are currently testing new code review model(s) that may lead to higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Outside diff range and nitpick comments (13)
crates/katana/runner/macro/Cargo.toml (1)
10-13: Ohayo once more, sensei! Dependencies are looking good!The dependencies are well-chosen for developing a procedural macro:
- proc-macro2, quote, and syn are the standard toolkit for this task.
- Versions are up-to-date, which is great for security and feature availability.
- The syn crate is correctly configured with additional features.
Consider using workspace dependencies for consistency:
[dependencies] -proc-macro2 = "1.0.86" -quote = "1.0" +proc-macro2.workspace = true +quote.workspace = true syn = { version = "2.0", features = [ "fold", "full" ] }This change would make it easier to manage versions across your workspace.
crates/katana/runner/macro/src/lib.rs (2)
1-7: Ohayo, sensei! LGTM with a small suggestion.The module structure and documentation look good. Clear attribution is provided for the implementation source.
Consider adding a brief description of what this macro does and its purpose in the project. This will help other developers understand the macro's functionality at a glance.
11-14: Ohayo, sensei! The macro implementation looks solid.The
testmacro is well-defined and correctly delegates processing to theentry::testfunction. TheTokenStreamconversions are handled appropriately.For improved clarity, consider adding a brief inline comment explaining the purpose of the
.into()conversions. This will help other developers understand the type conversions happening here.pub fn test(args: TokenStream, input: TokenStream) -> TokenStream { // Convert proc_macro::TokenStream to proc_macro2::TokenStream and back entry::test(args.into(), input.into()).into() }crates/katana/runner/tests/runner.rs (2)
9-12: Ohayo, sensei! The function structure looks good, but it lacks actual test logic.The
with_returntest function is correctly set up:
- It uses the
katana_runner::testattribute macro.- The function signature properly includes a
Resultreturn type.However, the function doesn't perform any actual testing or assertions. If this is intended as a placeholder, consider adding a TODO comment. Otherwise, it might be beneficial to add some test logic or remove the function if it's not needed.
Consider one of the following options:
- Add test logic:
#[katana_runner::test] fn with_return(runner: &RunnerCtx) -> Result<(), Box<dyn std::error::Error>> { // Add your test logic here assert!(runner.accounts().len() > 0, "Expected at least one account"); Ok(()) }
- Add a TODO comment if it's a placeholder:
#[katana_runner::test] fn with_return(_: &RunnerCtx) -> Result<(), Box<dyn std::error::Error>> { // TODO: Implement test logic Ok(()) }
- Remove the function if it's not needed.
14-19: Ohayo, sensei! The async test setup looks great, but let's add an assertion for completeness.The
with_asynctest function is well-structured:
- It correctly uses both
tokio::testandkatana_runner::testattribute macros for async testing.- The function properly retrieves the provider and calls the
chain_id()method.However, the retrieved chain ID is not used or asserted. To make this test more meaningful, consider adding an assertion for the chain ID.
Here's a suggested improvement:
#[tokio::test] #[katana_runner::test] async fn with_async(ctx: &RunnerCtx) -> Result<(), Box<dyn std::error::Error>> { let provider = ctx.provider(); let chain_id = provider.chain_id().await?; assert_eq!(chain_id, "SN_GOERLI", "Expected Starknet Goerli testnet"); Ok(()) }This addition ensures that the test is actually verifying the expected chain ID, making it more robust and informative.
bin/sozo/tests/test_account.rs (1)
33-35: Ohayo, sensei! Great improvements to the test setup!The changes to the
test_account_fetchfunction look good:
- Using
#[tokio::test]enables asynchronous testing with Tokio.- The new
#[katana_runner::test]attribute provides more control over the test environment.- Adding the
runner: &RunnerCtxparameter allows access to the runner context within the test.These changes enhance the testing capabilities and align well with the project's evolution.
Consider adding a brief comment explaining the purpose of the
accounts = 2andfee = falsesettings in thekatana_runner::testattribute. This would help other developers understand the test setup more quickly.bin/sozo/src/commands/options/account/mod.rs (2)
215-217: Ohayo, sensei! LGTM with a small suggestion.The changes to the test attributes and function signature look good:
- Using
#[tokio::test]provides better async support.- The
#[katana_runner::test]attribute allows for specific test configuration.- Adding the
runnerparameter enables access to the runner context.Consider adding a brief comment explaining the purpose of the
accounts = 2, fee = falseconfiguration for better clarity:/// Test with 2 accounts and fee disabled #[katana_runner::test(accounts = 2, fee = false)]
240-242: Ohayo again, sensei! LGTM with the same suggestion as before.The changes to this test function are consistent with the previous one, which is good for maintaining uniformity in the testing approach.
For consistency, consider adding the same brief comment here as suggested for the previous test:
/// Test with 2 accounts and fee disabled #[katana_runner::test(accounts = 2, fee = false)]crates/katana/runner/macro/src/item.rs (1)
27-31: Consider documenting the returnedBodystruct inbodymethod.While the
bodymethod is clear, adding documentation for theBodystruct it returns would enhance code readability and assist other developers in understanding its usage.crates/katana/runner/macro/src/config.rs (3)
101-102: Ohayo, sensei! Validate thatblock_timeis non-negative.While parsing
block_time, ensure that the value provided is non-negative to avoid unexpected behavior during runtime.You can add a check after parsing:
let block_time = parse_int(block_time, span, "block_time")? as u64; +if block_time < 0 { + return Err(syn::Error::new(span, "`block_time` must be non-negative.")); +} self.block_time = Some(block_time);Note: Since
block_timeis cast tou64, negative values will wrap around, so it's important to enforce this constraint.
131-147: Ohayo, sensei! Consider handling additional attributes inRunnerArg.Currently, the
RunnerArgenum and itsFromStrimplementation handle a fixed set of attributes. To enhance extensibility and user experience, consider allowing dynamic attribute handling or providing clearer error messages with suggestions when an unknown attribute is encountered.This could involve updating the error message to include all possible attributes or implementing a more flexible parsing mechanism.
149-202: Ohayo, sensei! Improve error handling for unexpected attribute formats.In the
build_configfunction, onlyNameValueattributes are handled, and any other format results in an error. Consider providing more specific error messages or supporting additional attribute formats to make the macro more user-friendly.crates/katana/runner/src/lib.rs (1)
256-256: Consistent use of absolute imports in test moduleOhayo, sensei! On line 256:
use crate::determine_default_program_path;Using an absolute path is acceptable, but for consistency and clarity, consider using a relative path if the module hierarchy allows it.
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 (15)
- Cargo.toml (2 hunks)
- bin/sozo/src/commands/options/account/mod.rs (3 hunks)
- bin/sozo/tests/test_account.rs (2 hunks)
- crates/katana/runner/Cargo.toml (1 hunks)
- crates/katana/runner/macro/Cargo.toml (1 hunks)
- crates/katana/runner/macro/src/config.rs (1 hunks)
- crates/katana/runner/macro/src/entry.rs (1 hunks)
- crates/katana/runner/macro/src/item.rs (1 hunks)
- crates/katana/runner/macro/src/lib.rs (1 hunks)
- crates/katana/runner/macro/src/utils.rs (1 hunks)
- crates/katana/runner/runner-macro/Cargo.toml (0 hunks)
- crates/katana/runner/runner-macro/src/lib.rs (0 hunks)
- crates/katana/runner/src/lib.rs (5 hunks)
- crates/katana/runner/src/prefunded.rs (1 hunks)
- crates/katana/runner/tests/runner.rs (1 hunks)
Files not reviewed due to no reviewable changes (2)
- crates/katana/runner/runner-macro/Cargo.toml
- crates/katana/runner/runner-macro/src/lib.rs
Additional comments not posted (24)
crates/katana/runner/macro/Cargo.toml (3)
1-4: Ohayo, sensei! Package metadata looks sharp!The package metadata is well-structured and follows good practices:
- Using workspace for edition and version ensures consistency across the project.
- The package name "katana-runner-macro" is clear and follows naming conventions.
7-8: Ohayo again, sensei! Library configuration is on point!The library is correctly configured as a procedural macro:
- Setting
proc-macro = trueis the right way to define a procedural macro crate.- This configuration aligns perfectly with the package name and its intended purpose.
15-16: Ohayo, sensei! A quick question about the dev dependencies.Good job using the workspace version for the starknet dependency. It ensures consistency across your project.
However, I'm curious about the inclusion of starknet as a development dependency for a procedural macro crate. Could you clarify its purpose here? Is it used in tests or examples?
To help understand this, let's check if there are any test files using starknet:
This will help us verify if starknet is indeed necessary as a dev dependency.
crates/katana/runner/macro/src/lib.rs (1)
9-9: Ohayo! This import looks perfect, sensei.The
TokenStreamimport fromproc_macrois exactly what we need for the macro implementation.crates/katana/runner/Cargo.toml (1)
9-10: Ohayo, sensei! Dependency updates look sharp!The changes to the dependencies are well-structured and align with the project's evolution:
- Adding
katana-node-bindingsas a workspace dependency streamlines version management.- Including
katana-runner-macrowith a specific path maintains local development flexibility.These updates should improve maintainability and consistency across the project.
However, to ensure everything is in order, let's verify the changes:
Verification successful
Ohayo, sensei! Dependency updates have been successfully verified!
All changes to the dependencies are correctly implemented:
- The
katana-node-bindingsis properly defined in the rootCargo.tomlworkspace.- The new
katana-runner-macrodirectory exists.- The old
runner-macrodirectory has been removed.These updates enhance the project's maintainability and consistency.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the new macro directory and the removal of the old runner-macro directory. # Test 1: Check if the new macro directory exists if [ -d "crates/katana/runner/macro" ]; then echo "New macro directory exists." else echo "Error: New macro directory not found." fi # Test 2: Check if the old runner-macro directory has been removed if [ ! -d "crates/katana/runner/runner-macro" ]; then echo "Old runner-macro directory has been removed." else echo "Warning: Old runner-macro directory still exists." fi # Test 3: Check if katana-node-bindings is defined in the workspace grep -q "katana-node-bindings" ../../Cargo.toml && echo "katana-node-bindings is defined in the workspace." || echo "Warning: katana-node-bindings not found in the workspace."Length of output: 477
Script:
#!/bin/bash # Description: Locate all Cargo.toml files and check for katana-node-bindings in the workspace. # Find all Cargo.toml files in the repository find . -name "Cargo.toml" -print # Search for katana-node-bindings in each found Cargo.toml find . -name "Cargo.toml" -exec grep -H "katana-node-bindings" {} \;Length of output: 1996
Script:
#!/bin/bash # Description: Verify that katana-node-bindings is defined in the root Cargo.toml. # Check if root Cargo.toml exists if [ -f "./Cargo.toml" ]; then echo "Root Cargo.toml found." # Search for katana-node-bindings in root Cargo.toml grep -q "katana-node-bindings" ./Cargo.toml && echo "katana-node-bindings is defined in the root workspace." || echo "Error: katana-node-bindings not found in the root workspace." else echo "Error: Root Cargo.toml not found." fiLength of output: 243
crates/katana/runner/tests/runner.rs (2)
4-7: Ohayo, sensei! LGTM! Well-structured test function.The
simpletest function is well-implemented:
- It effectively uses the
katana_runner::testattribute macro with custom parameters.- The assertion correctly checks that the number of accounts matches the specified parameter.
- The test is concise and focused on a single aspect of the
RunnerCtx.Great job on writing a clear and purposeful test!
1-19: Ohayo, sensei! Overall, this test file is well-structured and demonstrates good testing practices.The test file showcases:
- Consistent use of the
katana_runner::testattribute macro.- A mix of synchronous and asynchronous tests.
- Demonstration of custom parameters in the attribute macro.
- Coverage of different aspects of the
RunnerCtxfunctionality.Great job on creating a diverse set of tests! To further improve:
- Consider adding more assertions in the
with_returnandwith_asyncfunctions.- If possible, add more test cases to cover edge cases or different scenarios.
Keep up the excellent work, sensei!
bin/sozo/tests/test_account.rs (1)
6-6: Ohayo, sensei! LGTM on the new import!The new import
use katana_runner::RunnerCtx;is correctly added to support the changes in thetest_account_fetchfunction. It's properly placed with other imports.crates/katana/runner/macro/src/utils.rs (2)
4-10: Ohayo, sensei! This string parsing looks sharp!The
parse_stringfunction handles bothLit::StrandLit::Verbatimcases effectively. The error handling is clear and informative. Great job on including thefieldcontext in the error message!
44-46: Ohayo, sensei! This attribute checking function is a masterpiece of brevity!The
attr_ends_withfunction is concise, efficient, and correctly implements the desired functionality. It effectively uses the iterator'slast()method and makes an appropriate comparison withSome(segment). Well done!Cargo.toml (2)
32-32: Ohayo, sensei! The runner-macro path has been updated.The path for the
runner-macromember has been changed from"crates/katana/runner/runner-macro"to"crates/katana/runner/macro". This change suggests a restructuring of the project directories. It's a small but important update that keeps the project organized.
89-89: A new katana-node-bindings member has joined our dojo, sensei!The addition of
katana-node-bindings = { path = "crates/katana/node-bindings" }to the workspace dependencies is an exciting development. This new member could bring enhanced functionality or integration capabilities to the Katana runner. It aligns well with the PR objective of improving the Katana runner macro.bin/sozo/src/commands/options/account/mod.rs (1)
154-154: Ohayo, sensei! LGTM: New import for RunnerCtx.The addition of
use katana_runner::RunnerCtx;is consistent with the changes made in the test functions. It's necessary for the newrunnerparameter.crates/katana/runner/macro/src/entry.rs (3)
13-32: Ohayo, sensei! Excellent work on introducing thetestprocedural macro.The
testfunction effectively parses the input function and attribute arguments, handling errors gracefully to maintain IDE functionality even when parsing fails.
34-37: Great implementation of error handling intoken_stream_with_error.Extending the token stream with the compile error ensures that developers receive detailed feedback without interrupting the macro expansion process.
93-93: Good job ensuring the#[test]attribute is not duplicated.Checking for an existing
#[test]attribute prevents the test from running multiple times, which maintains correct test behavior.crates/katana/runner/macro/src/item.rs (4)
11-19: StructItemFnis well-defined and clear.The
ItemFnstruct appropriately encapsulates all components of a function item needed for procedural macro manipulation.
21-25: Efficient attribute iteration inattrsmethod.The
attrsmethod correctly combines outer and inner attributes, providing a seamless way to iterate over all attributes of the function item.
73-83: Correct implementation ofToTokenstrait forItemFn.The
ToTokenstrait is properly implemented, ensuring thatItemFncan be seamlessly converted into a token stream.
126-140: Well-implementedBodystruct and itsToTokenstrait.The
Bodystruct effectively represents the function body, and theToTokensimplementation correctly generates the token stream.crates/katana/runner/src/lib.rs (4)
13-13: Importingtestmacro fromkatana_runner_macroOhayo, sensei! Great job importing the
testmacro with:pub use katana_runner_macro::test;This enhances our testing suite by leveraging procedural macros.
23-38: IntroducingRunnerCtxstruct and implementingDereftraitOhayo, sensei! The addition of the
RunnerCtxstruct wrappingKatanaRunneris a smart move to provide context. Implementing theDereftrait allows seamless access to the underlyingKatanaRunner, enhancing code readability and usability.
202-245: Enhancing account management methods inKatanaRunnerOhayo, sensei! The new methods for account management:
accounts_data()accounts()account_data(index)account(index)account_to_single_owned(account)significantly improve the usability of
KatanaRunner. They provide convenient ways to access and interact with account data, which will streamline testing and development.
141-145: Verify proper handling ofstdoutwhen starting Katana instanceThere's a potential issue when attempting to take
stdoutfrom the child process:let stdout = instance.child_mut().stdout.take().context("failed to take subprocess stdout")?;If
stdoutisNone, this will lead to a runtime error. Ensure that the child process'sstdoutis properly initialized and not already taken elsewhere.To confirm if
stdoutis properly initialized and not taken previously, please run the following script:This will help identify if
stdout.take()is called multiple times, which could causestdoutto beNone.Verification successful
Ohayo, sensei!
stdoutHandling Verified incrates/katana/runner/src/lib.rsThe
stdout.take()method is called only once incrates/katana/runner/src/lib.rs, ensuring thatstdoutis properly initialized and not taken multiple times.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for multiple takes of stdout in the codebase. # Test: Search for all occurrences where stdout is taken. Expect: stdout.take() is called only once. rg --type rust 'stdout\.take\(\)'Length of output: 275
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
crates/katana/runner/src/lib.rs (2)
140-141: Ohayo, sensei! LGTM with a small suggestion: Updated Katana instance initialization.The change from
starttospawnand renamingkatanatoinstanceimproves clarity and potentially changes how the Katana instance is initialized. This aligns with the PR objectives of enhancing the runner macro.Consider adding a brief comment explaining the reason for using
spawninstead ofstart, if there's a significant difference in behavior.Also applies to: 144-144, 148-148
163-166: Ohayo, sensei! LGTM with a small suggestion: UpdatedKatanaRunnerinitialization.The changes to the
KatanaRunnerstruct initialization reflect the renaming ofkatanatoinstanceand add a newcontractfield. This aligns with the PR objectives of improving the runner functionality.Consider adding a brief comment explaining the purpose of the
contractfield and why it's initialized as aMutex<Option<None>>.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- crates/katana/runner/src/lib.rs (4 hunks)
- crates/katana/runner/src/prefunded.rs (0 hunks)
Files not reviewed due to no reviewable changes (1)
- crates/katana/runner/src/prefunded.rs
Additional comments not posted (4)
crates/katana/runner/src/lib.rs (4)
12-12: Ohayo, sensei! LGTM: New test macro import.The addition of
pub use katana_runner_macro::test;aligns well with the PR objectives, introducing a new testing macro for improved flexibility in function annotations.
13-14: Ohayo, sensei! LGTM: New imports for Starknet functionality.The addition of imports for Starknet accounts, types, and wallets is necessary for the new account management functionality in
KatanaRunner. This change supports the enhanced flexibility mentioned in the PR objectives.Also applies to: 17-17
22-37: Ohayo, sensei! LGTM: NewRunnerCtxstruct withDerefimplementation.The introduction of the
RunnerCtxstruct and itsDerefimplementation provides a clean way to encapsulate and access theKatanaRunnerinstance. This aligns well with the PR objectives, enhancing flexibility in handling the runner instance.
250-254: Ohayo, sensei! LGTM: Improveddetermine_default_program_pathfunction.The refactoring of
determine_default_program_pathto use a more concise if-else expression improves readability while maintaining the same functionality. This change, although minor, contributes to the overall code quality improvements in this PR.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
crates/katana/runner/src/lib.rs (1)
22-37: Ohayo, sensei! LGTM with a small suggestion: NewRunnerCtxstruct.The introduction of
RunnerCtxenhances flexibility in accessing theKatanaRunnerinstance, which aligns well with the PR objectives. TheDerefimplementation is a nice touch for seamless access.Consider adding a brief doc comment to the
RunnerCtxstruct explaining its purpose and relationship toKatanaRunner. This would improve the code's self-documentation:/// A wrapper around `KatanaRunner` that provides convenient access to the runner instance. #[derive(Debug)] pub struct RunnerCtx(KatanaRunner);
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- crates/katana/runner/macro/src/config.rs (1 hunks)
- crates/katana/runner/macro/src/lib.rs (1 hunks)
- crates/katana/runner/src/lib.rs (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- crates/katana/runner/macro/src/config.rs
- crates/katana/runner/macro/src/lib.rs
Additional comments not posted (4)
crates/katana/runner/src/lib.rs (4)
12-17: Ohayo, sensei! LGTM: New imports and macro usage.The addition of the
testmacro re-export and new Starknet-related imports aligns well with the PR objectives. These changes lay the groundwork for the enhanced Katana runner macro functionality.
140-141: Ohayo, sensei! LGTM: Updates toKatanaRunner::setup_and_start.The changes to use
spawninstead ofstartand obtaining theendpoint_urlfrom theinstanceappear to be necessary adjustments, likely due to updates in the underlyingKatanaAPI. These modifications ensure that theKatanaRunnerremains compatible with the latest version ofKatana.Also applies to: 144-148, 163-166
Line range hint
255-268: Ohayo, sensei! LGTM: New test fordetermine_default_program_path.The addition of a test for the
determine_default_program_pathfunction is a welcome improvement. It enhances the overall test coverage of the module and ensures that the function behaves correctly under different environment configurations. This aligns with good software engineering practices and contributes to the robustness of the Katana runner.
Line range hint
1-268: Ohayo, sensei! Overall LGTM with one important suggestion for improvement.The changes in this PR significantly enhance the Katana runner macro, aligning well with the stated objectives. The introduction of
RunnerCtx, the new account management methods, and the improvements to the test coverage all contribute to a more flexible and robust implementation.However, there's still one area that needs attention:
- Error handling in
account_to_single_owned: As mentioned earlier, consider refactoring this method to return aResultinstead of usingpanic!. This change would improve the robustness of the code and provide better error handling for callers.Once this issue is addressed, the PR will be in excellent shape for merging. Great work on these improvements, sensei!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2464 +/- ##
==========================================
+ Coverage 68.37% 68.42% +0.04%
==========================================
Files 365 368 +3
Lines 47957 48197 +240
==========================================
+ Hits 32790 32977 +187
- Misses 15167 15220 +53 ☔ View full report in Codecov by Sentry. |
tokioruntime in the expanded code, now you have to include#[tokio::test]manually. this would allow passing arguments to the proc macro per function basis.example:
the argument name be anything.
Summary by CodeRabbit
New Features
katana-node-bindingsmember for enhanced functionality.RunnerCtxstruct to streamline interactions with theKatanaRunner.KatanaRunner.Bug Fixes
Chores
runner-macropackage and its associated functionalities.