Skip to content
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

chore: add consensus to storage #872

Closed
wants to merge 1 commit into from

Conversation

renancloudwalk
Copy link
Contributor

No description provided.

@renancloudwalk renancloudwalk requested a review from a team as a code owner May 20, 2024 13:59
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

3, because the PR involves changes across multiple files and integrates a new feature (consensus) into the storage initialization process. The changes affect core functionalities and require careful review to ensure that the integration is handled correctly without introducing bugs or performance issues.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The use of .unwrap() in asynchronous code (consensus.sender.send("Block Saved.".to_string()).await.unwrap()) can cause the program to panic if the send operation fails. This should be handled more gracefully to prevent potential runtime crashes.

Possible Bug: The addition of None as a parameter in storage.init(None).await? across multiple files without corresponding changes in the function signature in some places could lead to issues if not updated everywhere consistently.

🔒 Security concerns

No

Code feedback:
relevant filesrc/eth/storage/stratus_storage.rs
suggestion      

Replace .unwrap() with proper error handling to prevent potential panics during runtime. Consider using .map_err() to handle errors gracefully. [important]

relevant lineconsensus.sender.send("Block Saved.".to_string()).await.unwrap(); //XXX temporary, later we will send the whole block

relevant filesrc/bin/importer_offline.rs
suggestion      

Ensure that all files where storage.init() is modified to include an optional Consensus parameter are updated to reflect this change in the function signature to maintain consistency and prevent runtime errors. [important]

relevant linelet storage = config.storage.init(None).await?;

relevant filesrc/bin/run_with_importer.rs
suggestion      

Consider checking the consensus object for errors or null values before using it to initialize storage. This can prevent issues where consensus might not be properly initialized. [medium]

relevant linelet storage = config.storage.init(Some(consensus.clone())).await?;

relevant filesrc/eth/consensus.rs
suggestion      

Consider encapsulating the sender within the Consensus struct to avoid direct access and manipulation from outside modules, enhancing encapsulation and safety. [medium]

relevant linepub sender: Sender, //XXX maybe we should privatize this and use a oneshot channel

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible issue
Replace unwrap with error handling in save_block

Replace the direct usage of unwrap in the save_block method with proper error handling to
prevent potential panics in production code.

src/eth/storage/stratus_storage.rs [337]

-consensus.sender.send("Block Saved.".to_string()).await.unwrap();
+if let Err(e) = consensus.sender.send("Block Saved.".to_string()).await {
+    eprintln!("Failed to send consensus message: {:?}", e);
+    return Err(anyhow!("Failed to send consensus message: {:?}", e));
+}
 
Suggestion importance[1-10]: 9

Why: This suggestion replaces the use of unwrap with proper error handling, which is crucial for preventing potential panics in production code. This enhances the robustness of the save_block method.

9
Improve error handling during storage initialization

Consider handling the error from the init method instead of using the ? operator, which
could cause the program to panic if an error occurs. This is especially important in
initialization code where recovery or alternative actions might be necessary.

src/bin/importer_offline.rs [50]

-let storage = config.storage.init(None).await?;
+let storage = match config.storage.init(None).await {
+    Ok(storage) => storage,
+    Err(e) => {
+        eprintln!("Failed to initialize storage: {:?}", e);
+        return Err(e);
+    }
+};
 
Suggestion importance[1-10]: 8

Why: This suggestion improves error handling by replacing the ? operator with a match statement, which prevents the program from panicking and allows for more graceful error handling. This is particularly important in initialization code.

8
Possible bug
Ensure Consensus readiness before initializing storage

When initializing storage with Some(consensus.clone()), ensure that the Consensus object
is fully configured and operational to avoid runtime issues where the consensus might not
be ready or properly configured.

src/bin/run_with_importer.rs [26]

-let storage = config.storage.init(Some(consensus.clone())).await?;
+if consensus.is_ready() {
+    let storage = config.storage.init(Some(consensus.clone())).await?;
+} else {
+    return Err(anyhow!("Consensus is not ready"));
+}
 
Suggestion importance[1-10]: 7

Why: This suggestion ensures that the Consensus object is fully configured before initializing storage, which can prevent runtime issues. However, it assumes the existence of an is_ready method, which may not be present.

7
Enhancement
Encapsulate the sender field to control its access

Consider encapsulating the sender field in the Consensus struct to control access and
prevent misuse, which can enhance encapsulation and maintainability.

src/eth/consensus.rs [33]

-pub sender: Sender<String>, //XXX maybe we should privatize this and use a oneshot channel
+private sender: Sender<String>,
 
+pub fn send_message(&self, message: String) -> Result<(), SendError<String>> {
+    self.sender.send(message)
+}
+
Suggestion importance[1-10]: 6

Why: Encapsulating the sender field improves encapsulation and maintainability by controlling access to the field. However, the suggestion introduces a new method which may require additional changes in the codebase to accommodate.

6

@mayconamaroCW mayconamaroCW deleted the add-consensus-to-storage branch May 29, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant