feat(sozo): add sozo auth back #2659
Conversation
WalkthroughOhayo, sensei! This pull request introduces several changes across various files, primarily focusing on enhancing command functionality, managing dependencies, and refining utility methods. Key modifications include the addition of new commands and structures for authentication, improvements to transaction options, and the introduction of new utility functions. Additionally, a new dependency is declared in 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: 0
🧹 Outside diff range and nitpick comments (9)
bin/sozo/Cargo.toml (1)
13-13: Consider adding a comment explaining the dependency's purpose, sensei!Since this dependency is part of the auth feature changes, it would be helpful to add a brief comment explaining its role in the authentication system.
+# Required for Sozo auth functionality cainome.workspace = truecrates/sozo/ops/src/migration_ui.rs (2)
35-42: Ohayo! The new method looks clean, sensei! 🎋The implementation follows good practices and maintains consistency with the existing codebase. However, consider these improvements for enhanced robustness:
- Add validation for empty frames vector
- Consider making the interval configurable
Here's a suggested enhancement:
pub fn new_with_frames(text: &'static str, frames: Vec<&'static str>) -> Self { + if frames.is_empty() { + return Self::new(Some(text)); + } let frames = spinners::SpinnerFrames { interval: 500, frames: frames.into_iter().collect() }; let spinner = Spinner::new(frames.clone(), text, None); Self { spinner, default_frames: frames, silent: false } }Also consider adding a variant that accepts the interval:
pub fn new_with_frames_and_interval( text: &'static str, frames: Vec<&'static str>, interval: u64 ) -> Self
35-35: Documentation could be more descriptive, sensei! 📚Consider enhancing the documentation to include:
- Expected format of frames
- Default interval value
- Example usage
Here's a suggested improvement:
- /// Returns a new instance with the given frames. + /// Returns a new instance with custom spinner frames. + /// + /// # Arguments + /// * `text` - The text to display alongside the spinner + /// * `frames` - Vector of strings representing animation frames (e.g., ["⌛", "⏳"]) + /// + /// # Example + /// ``` + /// let ui = MigrationUi::new_with_frames("Loading...", vec!["⌛", "⏳"]); + /// ``` + /// + /// Note: Uses a fixed interval of 500ms between frames.bin/sozo/src/commands/execute.rs (1)
81-88: Verify integration with auth system.The contract retrieval logic supports the new auth system by providing a unified way to resolve contracts from both manifest and chain state. This aligns well with the PR's objective of enhancing permission management.
Consider documenting the relationship between this contract resolution mechanism and the auth system in the module-level documentation, as it plays a crucial role in permission management.
bin/sozo/src/commands/mod.rs (1)
37-38: Consider command ordering in documentation.The Auth command is placed first in the enum, which affects the order in generated documentation. Consider maintaining alphabetical ordering or grouping related commands together for better documentation structure.
crates/dojo/types/src/naming.rs (1)
187-201: Well-structured test coverage, sensei!The test cases thoroughly verify both execution paths:
- Tag input path via
test_compute_selector_from_tag_or_name_tag- Name input path via
test_compute_selector_from_tag_or_name_nameHowever, consider adding one more test case for invalid input to ensure robust error handling.
Here's a suggested additional test:
#[test] fn test_compute_selector_from_tag_or_name_invalid() { // Should fall back to compute_bytearray_hash for invalid tags assert_eq!( compute_selector_from_tag_or_name("invalid@tag"), compute_bytearray_hash("invalid@tag") ); }bin/sozo/src/commands/options/transaction.rs (1)
Line range hint
186-251: Consider adding a simple Clone test.Ohayo! While the test coverage is comprehensive, consider adding a quick test to verify that cloning preserves all fields correctly. This would be especially relevant for the new auth clone functionality.
Here's a suggested test:
#[test] fn test_transaction_options_clone() { let opts = TransactionOptions { wait: true, receipt: true, fee: FeeToken::Strk, gas: Some(1000), gas_price: Some(100), max_fee_raw: None, fee_estimate_multiplier: None, walnut: false, }; let cloned = opts.clone(); assert_eq!(opts.wait, cloned.wait); assert_eq!(opts.gas, cloned.gas); assert_eq!(opts.gas_price, cloned.gas_price); // ... assert other fields }bin/sozo/src/utils.rs (2)
211-232: The contract retrieval implementation looks solid, sensei!The function effectively supports the new auth commands by providing a flexible way to retrieve contract information. However, consider adding documentation to explain:
- When to use
force_diffvs manifest- The expected contract format in the manifest
Add rustdoc comments explaining the parameters and return value:
/// Retrieves contract information either from the local manifest or from the world diff. /// /// # Arguments /// /// * `account` - Account options for chain interaction /// * `starknet` - Starknet configuration options /// * `world` - World configuration options /// * `ws` - Workspace reference /// * `force_diff` - When true, always fetch from chain regardless of manifest presence /// /// # Returns /// /// A HashMap mapping contract names to their ContractInfo
234-243: The confirmation prompt looks good, but could be more flexible, sensei!The implementation aligns well with the PR's safety requirements for the clone command. However, consider these enhancements:
pub fn prompt_confirm(prompt: &str) -> Result<bool> { print!("{} [y/N]", prompt); io::stdout().flush()?; let mut input = String::new(); io::stdin().read_line(&mut input)?; - Ok(input.trim().to_lowercase() == "y") + Ok(matches!(input.trim().to_lowercase().as_str(), "y" | "yes")) }Also consider adding an optional parameter for the default value:
pub fn prompt_confirm(prompt: &str, default: bool) -> Result<bool> { print!("{} [{}/{}]", prompt, if default { "Y/n" } else { "y/N" }); // ... rest of the implementation }
📜 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 (10)
bin/sozo/Cargo.toml(1 hunks)bin/sozo/src/commands/auth.rs(2 hunks)bin/sozo/src/commands/execute.rs(1 hunks)bin/sozo/src/commands/mod.rs(4 hunks)bin/sozo/src/commands/options/transaction.rs(1 hunks)bin/sozo/src/utils.rs(2 hunks)crates/dojo/types/src/naming.rs(2 hunks)crates/dojo/world/src/diff/resource.rs(1 hunks)crates/sozo/ops/src/auth.rs(0 hunks)crates/sozo/ops/src/migration_ui.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/sozo/ops/src/auth.rs
🔇 Additional comments (16)
bin/sozo/Cargo.toml (1)
13-13: Ohayo! Verify the usage of the new cainome dependency.
The dependency follows the workspace pattern consistently, but let's ensure it's being used.
✅ Verification successful
Ohayo! The cainome dependency is actively used in the auth module
The dependency is well-justified as it's being used in the auth-related code, specifically in bin/sozo/src/commands/auth.rs for handling ContractAddress types. The codebase shows extensive usage of cainome for Cairo serialization/deserialization across multiple modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that cainome is actually used in the codebase
# Expected: Find usage of cainome in the auth-related code
# Search for cainome usage in the source files
rg "use.*cainome"
# Search for specific cainome types or traits
rg "cainome::"
Length of output: 86635
crates/dojo/world/src/diff/resource.rs (2)
55-58: Ohayo! Clean and idiomatic implementation, sensei!
The new is_empty method is a well-crafted addition that aligns perfectly with the PR's permission management objectives. It follows Rust idioms by leveraging the standard HashSet::is_empty method and provides a clear, efficient way to check permission states.
55-58: Verify integration with auth commands, sensei!
Let's ensure this method is properly integrated with the new auth commands mentioned in the PR objectives.
✅ Verification successful
Ohayo! The implementation is correctly structured, sensei!
The is_empty() method is part of the DiffPermissions struct which handles permission comparisons between local and remote states. It's a utility method that complements other diff-related methods (only_local(), only_remote(), synced()). The earlier search results showed it's being used appropriately in auth-related contexts for permission checks.
The implementation is:
- Correctly handling both local and remote permission sets
- Used alongside other permission-related checks in auth flows
- Following the standard Rust empty check pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of is_empty in auth-related code
# Expected: Usage in auth list command implementation
# Search for potential usage of is_empty method
rg -p "is_empty.*DiffPermissions"
# Search specifically in auth-related files
fd -e rs auth --exec rg -p "is_empty" {}
Length of output: 582
Script:
#!/bin/bash
# Let's check the actual implementation and usage context of DiffPermissions
ast-grep --pattern 'struct DiffPermissions {
$$$
}'
# Also check for any direct usage of DiffPermissions::is_empty
rg -p "DiffPermissions.*is_empty" -A 2
# Let's see the file structure to understand the context better
fd -p "world.*diff.*resource.rs"
Length of output: 170
Script:
#!/bin/bash
# Let's check the content of the resource.rs file to understand the context
cat crates/dojo/world/src/diff/resource.rs
# Let's also check for any tests using this functionality
rg -p "test.*DiffPermissions" -A 5
Length of output: 4299
bin/sozo/src/commands/execute.rs (1)
81-88: Ohayo sensei! Consider optimizing parameter passing.
While the refactoring to use contracts_from_manifest_or_diff improves code clarity, we're unnecessarily cloning AccountOptions and StarknetOptions. Consider passing references instead.
- self.account.clone(),
- self.starknet.clone(),
+ &self.account,
+ &self.starknet,Let's verify the utility function implementation:
bin/sozo/src/commands/mod.rs (3)
4-4: LGTM! Clean integration of auth module.
Ohayo sensei! The auth module integration follows the established pattern perfectly, maintaining consistency with other module declarations.
Also applies to: 10-10
67-67: LGTM! Consistent Display implementation.
The Display implementation for the Auth command follows the established pattern perfectly.
92-92: LGTM! Verify auth command implementation.
The run function integration looks good, maintaining consistency with other commands. Let's verify the AuthArgs implementation:
✅ Verification successful
Auth command implementation is well-structured and complete, sensei! ✨
The implementation shows a robust auth command with:
- Proper error handling through Result type
- Complete command structure with Grant/Revoke/List/Clone operations
- Consistent async handling with tokio
- Well-organized permission management for writers and owners
The run function integration matches the codebase patterns perfectly, with proper workspace handling and profile configuration loading.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AuthArgs implementation matches PR objectives
# Test: Check if AuthArgs implements required commands
ast-grep --pattern 'impl AuthArgs {
$$$
fn run($_, $_) -> Result<()> {
$$$
}
$$$
}'
# Test: Verify command structure matches PR objectives
rg -A 5 'struct AuthArgs'
Length of output: 416
Script:
#!/bin/bash
# Let's check the AuthCommand enum and implementation
rg -A 10 'enum AuthCommand'
# Check the implementation of AuthArgs run method
ast-grep --pattern 'impl AuthArgs {
$$$
}'
# Check for any auth-related error handling
rg -A 5 'pub fn run.*Result' bin/sozo/src/commands/auth.rs
Length of output: 6388
crates/dojo/types/src/naming.rs (1)
95-101: Ohayo! Clean implementation of the selector computation function!
The new function elegantly handles both tag and name inputs while maintaining proper separation of concerns. It aligns well with the PR's auth enhancement objectives by providing a unified interface for selector computation.
bin/sozo/src/commands/options/transaction.rs (2)
9-9: Ohayo sensei! The Clone derive addition looks good!
The addition of Clone to TransactionOptions aligns well with the PR's objectives for cloning permissions and follows Rust best practices.
Line range hint 9-61: Verify auth command compatibility with transaction options.
The TransactionOptions struct appears well-equipped to handle the new auth commands, particularly the clone operation mentioned in the PR objectives. However, let's verify the usage in auth-related code.
✅ Verification successful
Ohayo sensei! Based on the verification results, I can now provide a conclusive response:
Transaction options are properly integrated with auth commands
The TransactionOptions struct is correctly integrated into the auth command implementation through the CommonAuthOptions struct, which includes it as a flattened field. This ensures all auth-related operations, including the new clone permissions feature, have access to the necessary transaction configuration options.
- Found in
bin/sozo/src/commands/auth.rs:TransactionOptionsis properly importedCommonAuthOptionsstruct includestransaction: TransactionOptions- Used by key auth functions like
clone_permissions,update_owners, andupdate_writers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how TransactionOptions is used in auth-related code
# Expected: Find usage in auth command implementations
# Search for TransactionOptions usage in auth-related files
rg -l "TransactionOptions" "bin/sozo/src/commands/auth"
# Search for specific auth command implementations
ast-grep --pattern 'fn $_(auth: AuthArgs, options: TransactionOptions) { $$$ }'
Length of output: 203
Script:
#!/bin/bash
# Let's try a broader search to find auth-related code and transaction options usage
# Find auth-related files
fd auth "bin/sozo/src"
# Search for TransactionOptions usage in the entire codebase
rg "use.*TransactionOptions" "bin/sozo/src" -A 2
# Search for auth command implementations with context
rg "fn.*auth.*Transaction" "bin/sozo/src" -A 5
# Look for auth-related structs and their fields
ast-grep --pattern 'struct $name {
$$$
options: TransactionOptions,
$$$
}'
Length of output: 1252
Script:
#!/bin/bash
# Let's examine the auth.rs file content to see how TransactionOptions is used
# Show the content of auth.rs with context
rg --type rust "struct|impl|fn" "bin/sozo/src/commands/auth.rs" -A 5
# Look for specific auth command implementations
ast-grep --pattern 'pub fn $_($_: $_Options, $_: TransactionOptions) {
$$$
}'
# Search for any auth-related command that uses TransactionOptions
rg "TransactionOptions" "bin/sozo/src/commands/auth.rs" -B 2 -A 2
Length of output: 4039
bin/sozo/src/utils.rs (1)
1-2: Ohayo sensei! The new imports look good!
The added imports are well-justified:
HashMapfor contract information storageWritefor user prompt handlingContractInfofor contract metadata management
Also applies to: 9-9
bin/sozo/src/commands/auth.rs (5)
105-130: Well-Structured Command Definitions
Ohayo, sensei! The expansion of the AuthCommand enum with the Grant, Revoke, List, and Clone commands is well-implemented. The use of CommonAuthOptions to consolidate shared options enhances code maintainability and readability. The detailed help messages and examples provided in the #[arg(help = "...")] attributes will greatly assist users.
193-200: Effective Error Handling for Identical Source and Target
Ohayo, sensei! The check for identical source and target addresses in the Clone command is a thoughtful addition. It prevents unintended operations and provides a clear error message to the user.
278-285: Confirm User Intent Before Proceeding
Ohayo, sensei! Prompting the user to confirm the permissions to be cloned enhances the safety of the clone_permissions function. The use of colored output makes the information clear and user-friendly.
573-624: Robust Implementation of PermissionPair Parsing
Ohayo, sensei! The PermissionPair struct and its implementations of to_selector_and_address and FromStr are well-designed. They provide a clean way to parse and handle permission pairs. The error messages are informative, guiding users to provide correct input.
626-686: Comprehensive Unit Tests for PermissionPair
Ohayo, sensei! The unit tests thoroughly cover various scenarios for the PermissionPair struct. They validate both successful parsing and error conditions, ensuring the reliability of the permission handling logic.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2659 +/- ##
==========================================
- Coverage 57.85% 57.49% -0.36%
==========================================
Files 399 400 +1
Lines 49800 50276 +476
==========================================
+ Hits 28810 28905 +95
- Misses 20990 21371 +381 ☔ View full report in Codecov by Sentry. |
Sozo auth has the following features:
The most basic command is very granular, and can allow the modification of permissions one by one.
A new command is available to list all the permissions in the world or for a specific resource only.
Finally, to ease the transition from developer to multisig ownership, a new
clonecommand allows to clone all the permission of a source contract address to a target contract address. Optionally, the source permissions can be revoked in the same transaction:This latest command is gated with prompts to the user to ensure the clone is not done by mistake.
Summary by CodeRabbit
Release Notes
New Features
Authcommand for managing contract permissions, including granting, revoking, listing, and cloning permissions.MigrationUiinstance with custom spinner frames.Enhancements
DiffPermissionsstruct.Bug Fixes
These updates enhance the functionality and usability of the application while maintaining existing features.