fix: fix sozo issues with migration on sepolia #2398
Conversation
WalkthroughOhayo, sensei! This pull request introduces various changes across multiple files, including updates to configuration files, enhancements in argument handling, and improvements in private key management. New dependencies are added, and existing imports are refactored. Additionally, new functions for password handling are introduced, and function parameters are optimized for better performance. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Keystore
participant Environment
User->>CLI: Input signer key
alt Signer key provided
CLI->>Environment: Retrieve signer key
else Signer key not provided
CLI->>Keystore: Prompt for password
Keystore->>CLI: Return private key
end
CLI->>User: Signer key ready
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional comments not posted (1)
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}
| @@ -4,10 +4,12 @@ use std::io::BufReader; | |||
| use std::path::PathBuf; | |||
|
|
|||
| use clap::Parser; | |||
| use dojo_utils::keystore::prompt_password_if_needed; | |||
There was a problem hiding this comment.
Refactor suggestion: Consider modularizing keystore handling.
The import of prompt_password_if_needed directly in the global scope suggests that password prompting and keystore handling are significant aspects of this module. Consider encapsulating these functionalities into a separate module or class to improve modularity and maintainability.
| /// Retrieves the signer from the CLI or environment metadata. | ||
| /// First, attempt to locate the signer from CLI arguments or environment variables via CLAP. | ||
| /// If unsuccessful, then search for the signer within the Dojo environment metadata. | ||
| /// If the signer is not found in any of the above locations, return an error. | ||
| pub fn signer(&self, env_metadata: Option<&Environment>, no_wait: bool) -> Result<LocalWallet> { | ||
| if let Some(private_key) = self.private_key(env_metadata) { | ||
| trace!(private_key, "Signing using private key."); | ||
| return Ok(LocalWallet::from_signing_key(SigningKey::from_secret_scalar( | ||
| Felt::from_str(&private_key)?, | ||
| ))); | ||
| let pk_cli = self.private_key.clone(); | ||
| let pk_env = env_metadata.and_then(|env| env.private_key().map(|s| s.to_string())); | ||
|
|
||
| let pk_keystore_cli = self.private_key_from_keystore_cli(no_wait)?; | ||
| let pk_keystore_env = self.private_key_from_keystore_env(env_metadata, no_wait)?; | ||
|
|
||
| let private_key = if let Some(private_key) = pk_cli { | ||
| trace!("Signing using private key from CLI."); | ||
| SigningKey::from_secret_scalar(Felt::from_str(&private_key)?) | ||
| } else if let Some(private_key) = pk_keystore_cli { | ||
| trace!("Signing using private key from CLI keystore."); | ||
| private_key | ||
| } else if let Some(private_key) = pk_env { | ||
| trace!("Signing using private key from env metadata."); | ||
| SigningKey::from_secret_scalar(Felt::from_str(&private_key)?) | ||
| } else if let Some(private_key) = pk_keystore_env { | ||
| trace!("Signing using private key from env metadata keystore."); | ||
| private_key | ||
| } else { | ||
| return Err(anyhow!( | ||
| "Could not find private key. Please specify the private key or path to the \ | ||
| keystore file." | ||
| )); | ||
| }; | ||
|
|
||
| Ok(LocalWallet::from_signing_key(private_key)) | ||
| } |
There was a problem hiding this comment.
Refactor suggestion: Improve clarity and modularity in signer method.
The signer method has been significantly refactored to streamline the process of obtaining a private key from various sources. While the multi-source approach enhances robustness, the method's complexity could be reduced by further modularizing the private key retrieval logic into separate methods or even a dedicated class. This would improve readability and maintainability.
| /// Retrieves the private key from the CLI keystore. | ||
| /// If the keystore path is not set, it returns `None`. | ||
| pub fn private_key_from_keystore_cli( | ||
| &self, | ||
| env_metadata: Option<&Environment>, | ||
| no_wait: bool, | ||
| ) -> Result<Option<SigningKey>> { | ||
| if let Some(path) = &self.keystore_path { | ||
| let maybe_password = if self.keystore_password.is_some() { | ||
| self.keystore_password.as_deref() | ||
| } else { | ||
| env_metadata.and_then(|env| env.keystore_password()) | ||
| }; | ||
|
|
||
| let password = prompt_password_if_needed(maybe_password, no_wait)?; | ||
|
|
||
| let private_key = SigningKey::from_keystore(path, &password)?; | ||
| return Ok(Some(private_key)); | ||
| } | ||
|
|
||
| if let Some(path) = self.keystore_path(env_metadata) { | ||
| let password = { | ||
| if let Some(password) = self.keystore_password(env_metadata) { | ||
| password.to_owned() | ||
| } else if no_wait { | ||
| return Err(anyhow!("Could not find password. Please specify the password.")); | ||
| } else { | ||
| trace!("Prompting user for keystore password."); | ||
| rpassword::prompt_password("Enter password: ")? | ||
| } | ||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
Review of private_key_from_keystore_cli method.
This method effectively encapsulates the logic for fetching private keys from the CLI keystore. The use of prompt_password_if_needed for handling passwords is a good practice as it enhances security by not persisting passwords longer than necessary.
However, consider handling the scenario where the keystore path is provided but the file does not exist or is not accessible. This could improve the robustness of the method by providing clearer feedback to the user when errors occur.
Add error handling for missing or inaccessible keystore files:
+ if !Path::new(path).exists() {
+ return Err(anyhow!("Keystore file not found at the specified path: {}", path));
+ }Committable suggestion was skipped due to low confidence.
| /// Retrieves the signer from the CLI or environment metadata. | ||
| /// First, attempt to locate the signer from CLI arguments or environment variables via CLAP. | ||
| /// If unsuccessful, then search for the signer within the Dojo environment metadata. | ||
| /// If the signer is not found in any of the above locations, return an error. | ||
| pub fn signer(&self, env_metadata: Option<&Environment>, no_wait: bool) -> Result<LocalWallet> { | ||
| if let Some(private_key) = self.private_key(env_metadata) { | ||
| trace!(private_key, "Signing using private key."); | ||
| return Ok(LocalWallet::from_signing_key(SigningKey::from_secret_scalar( | ||
| Felt::from_str(&private_key)?, | ||
| ))); | ||
| let pk_cli = self.private_key.clone(); | ||
| let pk_env = env_metadata.and_then(|env| env.private_key().map(|s| s.to_string())); | ||
|
|
||
| let pk_keystore_cli = self.private_key_from_keystore_cli(env_metadata, no_wait)?; | ||
| let pk_keystore_env = self.private_key_from_keystore_env(env_metadata, no_wait)?; | ||
|
|
||
| let private_key = if let Some(private_key) = pk_cli { | ||
| trace!("Signing using private key from CLI."); | ||
| SigningKey::from_secret_scalar(Felt::from_str(&private_key)?) | ||
| } else if let Some(private_key) = pk_keystore_cli { | ||
| trace!("Signing using private key from CLI keystore."); | ||
| private_key | ||
| } else if let Some(private_key) = pk_env { | ||
| trace!("Signing using private key from env metadata."); | ||
| SigningKey::from_secret_scalar(Felt::from_str(&private_key)?) | ||
| } else if let Some(private_key) = pk_keystore_env { | ||
| trace!("Signing using private key from env metadata keystore."); | ||
| private_key | ||
| } else { | ||
| return Err(anyhow!( | ||
| "Could not find private key. Please specify the private key or path to the \ | ||
| keystore file." | ||
| )); | ||
| }; | ||
|
|
||
| Ok(LocalWallet::from_signing_key(private_key)) | ||
| } |
There was a problem hiding this comment.
Refactor and enhance error handling in signer method.
The signer method has been refactored to streamline the process of obtaining a private key from various sources. This multi-source approach is robust, but the complexity could be reduced by further modularizing the private key retrieval logic into separate methods, which has been done to some extent with private_key_from_keystore_cli and private_key_from_keystore_env.
However, the error message in lines 71-73 could be more specific about which sources were checked. This would aid in debugging and provide clearer feedback to the user.
Consider enhancing the error message to specify which sources were checked before failing, as follows:
- "Could not find private key. Please specify the private key or path to the keystore file."
+ "Could not find private key in CLI, environment metadata, or associated keystores. Please check your configurations."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.
| /// Retrieves the signer from the CLI or environment metadata. | |
| /// First, attempt to locate the signer from CLI arguments or environment variables via CLAP. | |
| /// If unsuccessful, then search for the signer within the Dojo environment metadata. | |
| /// If the signer is not found in any of the above locations, return an error. | |
| pub fn signer(&self, env_metadata: Option<&Environment>, no_wait: bool) -> Result<LocalWallet> { | |
| if let Some(private_key) = self.private_key(env_metadata) { | |
| trace!(private_key, "Signing using private key."); | |
| return Ok(LocalWallet::from_signing_key(SigningKey::from_secret_scalar( | |
| Felt::from_str(&private_key)?, | |
| ))); | |
| let pk_cli = self.private_key.clone(); | |
| let pk_env = env_metadata.and_then(|env| env.private_key().map(|s| s.to_string())); | |
| let pk_keystore_cli = self.private_key_from_keystore_cli(env_metadata, no_wait)?; | |
| let pk_keystore_env = self.private_key_from_keystore_env(env_metadata, no_wait)?; | |
| let private_key = if let Some(private_key) = pk_cli { | |
| trace!("Signing using private key from CLI."); | |
| SigningKey::from_secret_scalar(Felt::from_str(&private_key)?) | |
| } else if let Some(private_key) = pk_keystore_cli { | |
| trace!("Signing using private key from CLI keystore."); | |
| private_key | |
| } else if let Some(private_key) = pk_env { | |
| trace!("Signing using private key from env metadata."); | |
| SigningKey::from_secret_scalar(Felt::from_str(&private_key)?) | |
| } else if let Some(private_key) = pk_keystore_env { | |
| trace!("Signing using private key from env metadata keystore."); | |
| private_key | |
| } else { | |
| return Err(anyhow!( | |
| "Could not find private key. Please specify the private key or path to the \ | |
| keystore file." | |
| )); | |
| }; | |
| Ok(LocalWallet::from_signing_key(private_key)) | |
| } | |
| /// Retrieves the signer from the CLI or environment metadata. | |
| /// First, attempt to locate the signer from CLI arguments or environment variables via CLAP. | |
| /// If unsuccessful, then search for the signer within the Dojo environment metadata. | |
| /// If the signer is not found in any of the above locations, return an error. | |
| pub fn signer(&self, env_metadata: Option<&Environment>, no_wait: bool) -> Result<LocalWallet> { | |
| let pk_cli = self.private_key.clone(); | |
| let pk_env = env_metadata.and_then(|env| env.private_key().map(|s| s.to_string())); | |
| let pk_keystore_cli = self.private_key_from_keystore_cli(env_metadata, no_wait)?; | |
| let pk_keystore_env = self.private_key_from_keystore_env(env_metadata, no_wait)?; | |
| let private_key = if let Some(private_key) = pk_cli { | |
| trace!("Signing using private key from CLI."); | |
| SigningKey::from_secret_scalar(Felt::from_str(&private_key)?) | |
| } else if let Some(private_key) = pk_keystore_cli { | |
| trace!("Signing using private key from CLI keystore."); | |
| private_key | |
| } else if let Some(private_key) = pk_env { | |
| trace!("Signing using private key from env metadata."); | |
| SigningKey::from_secret_scalar(Felt::from_str(&private_key)?) | |
| } else if let Some(private_key) = pk_keystore_env { | |
| trace!("Signing using private key from env metadata keystore."); | |
| private_key | |
| } else { | |
| return Err(anyhow!( | |
| "Could not find private key in CLI, environment metadata, or associated keystores. Please check your configurations." | |
| )); | |
| }; | |
| Ok(LocalWallet::from_signing_key(private_key)) | |
| } |
| /// Retrieves the private key from the keystore in the environment metadata. | ||
| /// If the keystore path is not set, it returns `None`. | ||
| pub fn private_key_from_keystore_env( | ||
| &self, | ||
| env_metadata: Option<&Environment>, | ||
| no_wait: bool, | ||
| ) -> Result<Option<SigningKey>> { | ||
| if let Some(path) = env_metadata.and_then(|env| env.keystore_path()) { | ||
| let maybe_password = if self.keystore_password.is_some() { | ||
| self.keystore_password.as_deref() | ||
| } else { | ||
| env_metadata.and_then(|env| env.keystore_password()) | ||
| }; | ||
|
|
||
| let password = prompt_password_if_needed(maybe_password, no_wait)?; | ||
|
|
||
| let private_key = SigningKey::from_keystore(path, &password)?; | ||
| return Ok(LocalWallet::from_signing_key(private_key)); | ||
| return Ok(Some(private_key)); | ||
| } | ||
|
|
||
| Err(anyhow!( | ||
| "Could not find private key. Please specify the private key or path to the keystore \ | ||
| file." | ||
| )) | ||
| Ok(None) |
There was a problem hiding this comment.
Review of private_key_from_keystore_env method.
Similar to the CLI keystore method, this function handles the retrieval of private keys from the environment metadata keystore. The structure and error handling are consistent with the CLI method, which is good for maintainability.
As with the CLI method, adding checks for file existence and accessibility could prevent runtime errors and improve user experience by providing immediate feedback on misconfigurations.
Implement similar error handling as suggested for the CLI keystore method:
+ if !Path::new(path).exists() {
+ return Err(anyhow!("Keystore file not found at the specified path: {}", path));
+ }Committable suggestion was skipped due to low confidence.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2398 +/- ##
==========================================
- Coverage 68.43% 68.38% -0.06%
==========================================
Files 358 359 +1
Lines 47349 47445 +96
==========================================
+ Hits 32404 32444 +40
- Misses 14945 15001 +56 ☔ View full report in Codecov by Sentry. |
Ensures migration is more robust when some resources are already declared.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Scarb.tomlfile.Chores