feat(katana): initialize slot paymaster account
#3014
Conversation
|
Ohayo sensei! WalkthroughThis pull request introduces a new feature flag, Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Init CLI
participant Prompt as Prompt Function
participant Slot as Slot Module
participant Genesis as Genesis Generator
participant User as User
User->>CLI: Execute init command
CLI->>Prompt: Invoke prompt for account inputs
Prompt->>User: Request paymaster public key input (if init-slot enabled)
User->>Prompt: Provide paymaster public keys
Prompt->>CLI: Return slot paymaster data
CLI->>Slot: Parse and add slot paymasters
Slot->>Genesis: Update genesis state with paymaster accounts
CLI->>User: Display updated genesis outcome
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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: 1
🧹 Nitpick comments (2)
bin/katana/src/cli/init/slot.rs (1)
40-60: The implementation is solid, sensei! Consider adding documentation.The function efficiently handles paymaster account creation. Adding rustdoc comments would improve maintainability.
+/// Adds paymaster accounts to the genesis state. +/// +/// # Arguments +/// * `genesis` - Mutable reference to the genesis state +/// * `slot_paymasters` - Slice of paymaster account arguments +/// +/// # Returns +/// Vector of contract addresses for the added paymaster accounts pub fn add_paymasters_to_genesis( genesis: &mut Genesis, slot_paymasters: &[PaymasterAccountArgs], ) -> Vec<ContractAddress> {bin/katana/src/cli/init/prompt.rs (1)
128-137: Add validation for duplicate public keys, sensei!Consider checking for duplicate public keys to prevent unintended behavior.
let mut slot_paymasters = Vec::new(); + let mut seen_keys = std::collections::HashSet::new(); while Confirm::new("Add slot paymaster account?").with_default(true).prompt()? { let public_key = CustomType::<Felt>::new("Paymaster public key") .with_formatter(&|input: Felt| format!("{input:#x}")) .prompt()?; + if !seen_keys.insert(public_key) { + println!("Warning: Duplicate public key detected"); + continue; + } slot_paymasters.push(super::slot::PaymasterAccountArgs { public_key }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
bin/katana/Cargo.toml(1 hunks)bin/katana/src/cli/init/mod.rs(6 hunks)bin/katana/src/cli/init/prompt.rs(1 hunks)bin/katana/src/cli/init/slot.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (3)
bin/katana/src/cli/init/slot.rs (1)
14-24: Ohayo! The SlotArgs struct looks good, sensei!The struct is well-designed with appropriate constraints and documentation.
bin/katana/src/cli/init/mod.rs (1)
187-194: Clean implementation of genesis generation, sensei!The function nicely encapsulates the genesis generation logic with appropriate defaults.
bin/katana/Cargo.toml (1)
43-43: Feature configuration looks good, sensei!The init-slot feature is properly defined and included in the default features.
Also applies to: 46-46
bin/katana/src/cli/init/slot.rs
Outdated
| fn parse_paymaster_accounts_args(value: &str) -> Result<Vec<PaymasterAccountArgs>> { | ||
| let mut accounts = Vec::new(); | ||
| for s in value.split(',') { | ||
| accounts.push(PaymasterAccountArgs { public_key: Felt::from_str(s)? }); | ||
| } | ||
| Ok(accounts) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation, sensei!
The parser should validate:
- Empty input strings
- Malformed public keys
- Maximum number of paymaster accounts
fn parse_paymaster_accounts_args(value: &str) -> Result<Vec<PaymasterAccountArgs>> {
+ if value.trim().is_empty() {
+ return Err(anyhow::anyhow!("Empty input string"));
+ }
let mut accounts = Vec::new();
for s in value.split(',') {
+ let s = s.trim();
+ if s.is_empty() {
+ return Err(anyhow::anyhow!("Empty public key in list"));
+ }
accounts.push(PaymasterAccountArgs { public_key: Felt::from_str(s)? });
}
+ if accounts.len() > 10 { // Example limit
+ return Err(anyhow::anyhow!("Too many paymaster accounts"));
+ }
Ok(accounts)
}📝 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.
| fn parse_paymaster_accounts_args(value: &str) -> Result<Vec<PaymasterAccountArgs>> { | |
| let mut accounts = Vec::new(); | |
| for s in value.split(',') { | |
| accounts.push(PaymasterAccountArgs { public_key: Felt::from_str(s)? }); | |
| } | |
| Ok(accounts) | |
| } | |
| fn parse_paymaster_accounts_args(value: &str) -> Result<Vec<PaymasterAccountArgs>> { | |
| if value.trim().is_empty() { | |
| return Err(anyhow::anyhow!("Empty input string")); | |
| } | |
| let mut accounts = Vec::new(); | |
| for s in value.split(',') { | |
| let s = s.trim(); | |
| if s.is_empty() { | |
| return Err(anyhow::anyhow!("Empty public key in list")); | |
| } | |
| accounts.push(PaymasterAccountArgs { public_key: Felt::from_str(s)? }); | |
| } | |
| if accounts.len() > 10 { // Example limit | |
| return Err(anyhow::anyhow!("Too many paymaster accounts")); | |
| } | |
| Ok(accounts) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
bin/katana/src/cli/init/slot.rs (3)
26-30: Consider enhancing documentation, sensei!The struct could benefit from additional documentation:
- Valid format for public keys
- Any constraints or limitations
- Example usage
#[derive(Debug, Clone)] +/// Represents a paymaster account configuration. +/// +/// # Example +/// ``` +/// let paymaster = PaymasterAccountArgs { +/// public_key: Felt::from_str("0x1234...")?, +/// }; +/// ``` pub struct PaymasterAccountArgs { /// The public key of the paymaster account. + /// Must be a valid Starknet public key in hexadecimal format. pub public_key: Felt, }
40-60: Consider adding capacity validation and error handling, sensei!The function could benefit from:
- Validating the input slice capacity to prevent excessive memory allocation
- Adding error handling for potential overflow scenarios
pub fn add_paymasters_to_genesis( genesis: &mut Genesis, slot_paymasters: &[PaymasterAccountArgs], ) -> Vec<ContractAddress> { + const MAX_PAYMASTERS: usize = 1000; // Example limit + if slot_paymasters.len() > MAX_PAYMASTERS { + panic!("Too many paymaster accounts. Maximum allowed: {}", MAX_PAYMASTERS); + } + let mut accounts = Vec::with_capacity(slot_paymasters.len());
62-93: Enhance test coverage with edge cases, sensei!Consider adding tests for:
- Empty paymaster list
- Maximum capacity scenarios
- Error cases from parsing
#[test] fn test_empty_paymasters() { let mut genesis = Genesis::default(); let paymasters = Vec::new(); let addresses = add_paymasters_to_genesis(&mut genesis, &paymasters); assert!(addresses.is_empty()); } #[test] #[should_panic(expected = "Too many paymaster accounts")] fn test_max_capacity() { let mut genesis = Genesis::default(); let paymasters = vec![ PaymasterAccountArgs { public_key: Felt::from(1) }; 1001 // Exceeds MAX_PAYMASTERS ]; add_paymasters_to_genesis(&mut genesis, &paymasters); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/katana/src/cli/init/slot.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: clippy
- GitHub Check: ensure-wasm
- GitHub Check: build
- GitHub Check: docs
🔇 Additional comments (2)
bin/katana/src/cli/init/slot.rs (2)
14-24: Ohayo! The SlotArgs struct looks solid, sensei!The struct is well-structured with proper clap attributes and conditional requirements. The
requires_all = ["id", "slot"]constraint ensures proper configuration.
32-38: Add input validation and improve error context, sensei!The input validation concerns from the previous review are still valid.
Additionally, consider improving error messages with more context:
fn parse_paymaster_accounts_args(value: &str) -> Result<Vec<PaymasterAccountArgs>> { let mut accounts = Vec::new(); - for s in value.split(',') { - accounts.push(PaymasterAccountArgs { public_key: Felt::from_str(s)? }); + for (index, s) in value.split(',').enumerate() { + accounts.push(PaymasterAccountArgs { + public_key: Felt::from_str(s).map_err(|e| { + anyhow::anyhow!("Invalid public key at position {}: {}", index + 1, e) + })?, + }); } Ok(accounts) }
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3014 +/- ##
==========================================
+ Coverage 56.40% 56.42% +0.01%
==========================================
Files 434 435 +1
Lines 58077 58140 +63
==========================================
+ Hits 32761 32808 +47
- Misses 25316 25332 +16 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bin/katana/src/cli/init/slot.rs (1)
31-37:⚠️ Potential issueInput validation is still missing, sensei!
The previous review comment about adding input validation for the FromStr implementation is still relevant.
impl FromStr for PaymasterAccountArgs { type Err = anyhow::Error; fn from_str(s: &str) -> Result<Self> { + let s = s.trim(); + if s.is_empty() { + return Err(anyhow::anyhow!("Empty public key")); + } + if !s.starts_with("0x") { + return Err(anyhow::anyhow!("Public key must start with 0x")); + } Ok(PaymasterAccountArgs { public_key: Felt::from_str(s)? }) } }
🧹 Nitpick comments (1)
bin/katana/src/cli/init/slot.rs (1)
61-92: Let's enhance the test coverage, sensei!While the current test verifies the happy path, we should add more test cases:
- Empty paymaster list
- Maximum number of paymasters
- Error cases for invalid public keys
#[cfg(test)] mod tests { use super::*; + #[test] + fn test_empty_paymasters() { + let mut genesis = Genesis::default(); + let paymasters = Vec::new(); + let addresses = add_paymasters_to_genesis(&mut genesis, &paymasters); + assert!(addresses.is_empty()); + } + + #[test] + fn test_invalid_public_key() { + assert!(PaymasterAccountArgs::from_str("invalid").is_err()); + assert!(PaymasterAccountArgs::from_str("").is_err()); + assert!(PaymasterAccountArgs::from_str("123").is_err()); + } + #[test] fn test_add_paymasters_to_genesis() {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/katana/src/cli/init/slot.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ensure-wasm
- GitHub Check: build
- GitHub Check: docs
- GitHub Check: clippy
🔇 Additional comments (1)
bin/katana/src/cli/init/slot.rs (1)
39-59: Ohayo! The genesis account creation looks good, sensei!The implementation correctly follows the PR objectives by using the specified default class hash and creating deterministic addresses for the paymaster accounts.
| #[arg(requires_all = ["id", "slot"])] | ||
| #[arg(long = "slot.paymasters", value_delimiter = ',')] | ||
| pub paymaster_accounts: Option<Vec<PaymasterAccountArgs>>, |
There was a problem hiding this comment.
Ohayo sensei! The argument constraints need adjustment.
The requires_all = ["id", "slot"] constraint contradicts the PR objectives. According to the example usage katana init --slot --slot.paymasters 0x1,0x2,0x3, only the --slot flag is needed, not an id argument.
- #[arg(requires_all = ["id", "slot"])]
+ #[arg(requires = "slot")]
#[arg(long = "slot.paymasters", value_delimiter = ',')]
pub paymaster_accounts: Option<Vec<PaymasterAccountArgs>>,📝 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.
| #[arg(requires_all = ["id", "slot"])] | |
| #[arg(long = "slot.paymasters", value_delimiter = ',')] | |
| pub paymaster_accounts: Option<Vec<PaymasterAccountArgs>>, | |
| #[arg(requires = "slot")] | |
| #[arg(long = "slot.paymasters", value_delimiter = ',')] | |
| pub paymaster_accounts: Option<Vec<PaymasterAccountArgs>>, |
This is VERY
slot-specific addition. This PR adds new arguments in thekatana initcommand, essentially allowing to initialize # paymaster accounts in aslot-hostedkatana. As such, it is gated under theinit-slotfeature (but currently is enabled by default) to avoid having separate binary builds.To configure the paymasters, the
--slotflag must be specified.Usage:
, where
0x1,0x2,0x3are the public keys of the paymaster accounts. The number of public keys listed implicitly determine the total # of paymaster accounts to be pre-deployed.The accounts use the same class hash, and salt as the default predeployed accounts in
katana(as well as the master account in the default genesis created bykatana init), where:-Class hash:
0x07dc7899aa655b0aae51eadff6d801a58e97dd99cf4666ee59e704249e51adf2Salt:
0x666The contract addresses of the paymasters can be computed deterministically like so:
where h is the pedersen hash.
Summary by CodeRabbit
New Features
init-slotfeature for enhanced initialization options, allowing configuration of slot paymaster accounts during genesis setup.init-slotfunctionality.Chores