fix(katana): ensure paymaster accounts are unique #3017
Conversation
|
Ohayo, sensei! Here’s the detailed breakdown of the changes in this PR: WalkthroughThis pull request updates the CLI initialization and genesis account creation processes. The prompt function now uses Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant PF as Prompt Function
participant VM as Validator Closure
participant SP as SlotPaymasters (Rc<RefCell>)
U->>PF: Initiate slot paymaster prompt
PF->>SP: Initialize empty paymaster list
PF->>U: Prompt for public key (with count)
U->>PF: Provide public key
PF->>U: Prompt for salt input
U->>PF: Provide salt
PF->>VM: Validate uniqueness of public key-salt
VM-->>PF: Return validation result
PF->>SP: Append validated account
PF->>U: Confirm addition or re-prompt
sequenceDiagram
participant C as Caller
participant GA as GenesisAccount::new_with_salt_and_balance
participant NI as GenesisAccount::new_inner
C->>GA: Call new_with_salt_and_balance(pub_key, class_hash, salt, balance)
GA->>NI: Invoke new_inner(pub_key, class_hash, salt)
NI-->>GA: Return (ContractAddress, GenesisAccount)
GA-->>C: Return computed (ContractAddress, GenesisAccount)
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: 0
🧹 Nitpick comments (4)
bin/katana/src/cli/init/slot.rs (3)
38-60: Ohayo! Consider enhancing error messages for better user experience.The FromStr implementation could benefit from more descriptive error messages that include the expected format.
- let public_key = parts.next().ok_or_else(|| anyhow!("missing public key"))?; - let salt = parts.next().ok_or_else(|| anyhow!("missing salt"))?; + let public_key = parts.next().ok_or_else(|| anyhow!("missing public key - expected format: <public_key>,<salt>"))?; + let salt = parts.next().ok_or_else(|| anyhow!("missing salt - expected format: <public_key>,<salt>"))?;
124-155: Consider adding more edge cases to the test suite.The test effectively verifies address uniqueness, but could be enhanced with additional scenarios:
- Test with zero salt
- Test with maximum salt value
- Test with consecutive salt values
171-202: Consider adding error case tests.While the happy path is well tested, consider adding tests for:
- Invalid public key format
- Invalid salt format
- Missing salt value
bin/katana/src/cli/init/prompt.rs (1)
132-171: Nice use of Rc for state management sensei!The implementation effectively ensures unique paymaster accounts.
Consider extracting validation logic to a separate function.
The unique_salt_validator closure could be moved to a standalone function for better readability.
+fn is_unique_salt(paymasters: &[PaymasterAccountArgs], public_key: Felt, salt: &Felt) -> bool { + !paymasters.iter().any(|pm| pm.public_key == public_key && pm.salt == *salt) +} + let slot_paymasters_clone = slot_paymasters.clone(); let unique_salt_validator = move |salt: &Felt| { - let pred = |pm: &PaymasterAccountArgs| pm.public_key == public_key && pm.salt == *salt; - let duplicate = slot_paymasters_clone.borrow().iter().any(pred); + let unique = is_unique_salt(&slot_paymasters_clone.borrow(), public_key, salt); - if !duplicate { + if unique { Ok(Validation::Valid) } else { Ok(Validation::Invalid(ErrorMessage::Custom(
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/katana/src/cli/init/prompt.rs(4 hunks)bin/katana/src/cli/init/slot.rs(6 hunks)crates/katana/primitives/src/genesis/allocation.rs(1 hunks)
🔇 Additional comments (2)
bin/katana/src/cli/init/slot.rs (1)
62-87: LGTM sensei! Clean implementation of paymaster account creation.The function correctly utilizes the new salt-based account creation to ensure unique addresses.
crates/katana/primitives/src/genesis/allocation.rs (1)
214-228: Clean implementation of salt-based account creation sensei!The new methods follow the existing pattern and properly handle salt for unique address generation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
bin/katana/src/cli/init/slot.rs (2)
21-40: Ohayo! Update documentation to match current implementation.The example in the documentation shows comma as a delimiter (
0x1,0x2 0x3,0x4), but the code uses space delimiter as specified invalue_delimiter = ' '. Let's also add a note about uniqueness validation./// This argument accepts a list of values, where each value is a pair of public key and salt -/// separated by a comma. If more than one pair is provided, the double quotes are required to +/// separated by a space. If more than one pair is provided, the double quotes are required to /// prevent the shell from prematurely splitting the argument. +/// +/// Note: Each public key and salt combination must be unique to ensure unique paymaster accounts. /// /// For example: /// /// ``` -/// --slot.paymasters 0x1,0x2 +/// --slot.paymasters "0x1,0x2" /// ``` /// /// ``` -/// --slot.paymasters "0x1,0x2 0x3,0x4 0x5,0x6" +/// --slot.paymasters "0x1,0x2 0x1,0x3 0x2,0x1" /// ```
54-64: Consider validating for extra parts, sensei!The parsing looks good with proper error handling, but we should validate that there are no unexpected extra parts after the salt.
fn from_str(s: &str) -> Result<Self> { let mut parts = s.split(','); let public_key = parts.next().ok_or_else(|| anyhow!("missing public key"))?; let salt = parts.next().ok_or_else(|| anyhow!("missing salt"))?; + if parts.next().is_some() { + return Err(anyhow!("unexpected extra parts after salt")); + } let public_key = Felt::from_str(public_key)?; let salt = Felt::from_str(salt)?; Ok(PaymasterAccountArgs { public_key, salt }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/katana/src/cli/init/slot.rs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build
- GitHub Check: clippy
- GitHub Check: docs
- GitHub Check: ensure-wasm
🔇 Additional comments (2)
bin/katana/src/cli/init/slot.rs (2)
77-82: LGTM! Good use of salt for unique account generation.The implementation correctly uses
new_with_salt_and_balanceto ensure unique paymaster accounts by combining public key and salt.
129-206: Excellent test coverage, sensei!The new tests thoroughly validate:
- Uniqueness of addresses with same public key but different salts
- CLI argument parsing with and without paymaster args
- Edge cases with multiple paymasters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3017 +/- ##
==========================================
+ Coverage 56.21% 56.24% +0.02%
==========================================
Files 436 436
Lines 58737 58829 +92
==========================================
+ Hits 33021 33087 +66
- Misses 25716 25742 +26 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Tests