-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(katana): allow specifying multiple values w/o wrapping in quotes #3018
Conversation
WalkthroughOhayo, sensei! This PR updates the CLI for specifying multiple paymaster accounts in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant SlotArgs
User->>CLI: Run command with --slot.paymasters "account1 account2 ..."
CLI->>SlotArgs: Parse paymaster accounts using num_args = 1..
SlotArgs-->>CLI: Return list of parsed accounts
CLI-->>User: Process command with provided paymaster accounts
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
bin/katana/src/cli/init/slot.rs (2)
23-34
: Ohayo sensei! Consider enhancing the documentation with validation rules.The documentation clearly explains the new format, but it would be helpful to include:
- Valid format for public keys and salts (e.g., hex format, length)
- Error handling behavior for invalid inputs
188-190
: Ohayo sensei! Consider adding more test cases.While the current test covers the happy path well, consider adding test cases for:
- Edge cases (empty string, malformed input)
- Maximum number of arguments (if any)
- Invalid hex values
Example test case for invalid input:
#[test] fn test_parse_invalid_paymaster_args() { #[derive(Parser)] struct Cli { #[arg(long)] id: bool, #[command(flatten)] slot: SlotArgs, } // Test malformed input let result = Cli::parse_from([ "cli", "--id", "--slot", "--slot.paymasters", "invalid", ]); assert!(result.is_err()); // Test invalid hex let result = Cli::parse_from([ "cli", "--id", "--slot", "--slot.paymasters", "0xZ,0x1", ]); assert!(result.is_err()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/katana/src/cli/init/slot.rs
(2 hunks)
🔇 Additional comments (1)
bin/katana/src/cli/init/slot.rs (1)
36-36
: LGTM! The argument parsing change looks solid.The switch to
num_args = 1..
elegantly handles both single and multiple paymaster specifications without requiring quotes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3018 +/- ##
==========================================
- Coverage 56.24% 56.23% -0.01%
==========================================
Files 436 436
Lines 58829 58830 +1
==========================================
- Hits 33087 33084 -3
- Misses 25742 25746 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I've missed that also, better than the quotes. 👍
Accidentally remove the
num_args
after I wrote the test for it in #3017 . Now this should be possible:without quotes.
@glihm
Summary by CodeRabbit