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

Append entries endpoint #892

Merged
merged 7 commits into from
May 21, 2024
Merged

Append entries endpoint #892

merged 7 commits into from
May 21, 2024

Conversation

renancloudwalk
Copy link
Contributor

No description provided.

@renancloudwalk renancloudwalk requested a review from a team as a code owner May 21, 2024 22:07
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

2, because the PR involves a moderate amount of changes with the addition of a new RPC method and its corresponding implementation in the blockchain client. The changes are straightforward and localized to specific parts of the codebase.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The new RPC method stratus_append_entries might not handle different types of input or errors in the Params. It currently just returns a fixed response, which might not be adequate for all use cases.

🔒 Security concerns

No

Code feedback:
relevant filesrc/eth/rpc/rpc_server.rs
suggestion      

Consider adding error handling or input validation for the stratus_append_entries method to ensure it can manage different types of input and respond appropriately to erroneous data. [important]

relevant lineOk(json!(true))

relevant filesrc/infra/blockchain_client/client.rs
suggestion      

Ensure that the entries variable in append_entries method is properly serialized and handled in case of serialization errors. You might want to add error handling around the serde_json::to_value(entries)?; to manage serialization failures gracefully. [important]

relevant linelet entries = serde_json::to_value(entries)?;

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Best practice
Add detailed error handling for the append_entries request

Ensure that the error from the request method is properly handled. Currently, the method
call is followed by a ?, which will propagate the error, but it might be beneficial to add
specific error handling or logging to provide more context about the failure.

src/infra/blockchain_client/client.rs [92]

-self.http.request::<(), Vec<JsonValue>>("stratus_appendEntries", vec![entries]).await?;
+self.http.request::<(), Vec<JsonValue>>("stratus_appendEntries", vec![entries]).await
+    .map_err(|e| {
+        tracing::error!("Failed to append entries: {}", e);
+        e
+    })?;
 
Suggestion importance[1-10]: 9

Why: Adding detailed error handling and logging can significantly improve debugging and maintenance, making it easier to identify and resolve issues related to the append_entries request.

9
Enhancement
Suggest adding implementation details for stratus_append_entries

The function stratus_append_entries currently returns a static JSON value. If this
function is intended to handle more complex logic or return dynamic data, consider
implementing the necessary logic or indicating that further implementation is needed.

src/eth/rpc/rpc_server.rs [234]

+// Placeholder for actual implementation
+// TODO: Implement the logic for append entries
 Ok(json!(true))
 
Suggestion importance[1-10]: 7

Why: The suggestion correctly identifies that the function currently returns a static value and suggests adding a placeholder for future implementation, which can improve code clarity and future development.

7
Maintainability
Enhance logging for better traceability of the append entries process

Consider adding more detailed logging before making the HTTP request to append entries.
This can help in debugging and understanding the flow of data, especially when dealing
with complex data structures.

src/infra/blockchain_client/client.rs [90]

-tracing::debug!(?entries, "appending entries");
+tracing::debug!(?entries, "Preparing to append entries");
+tracing::debug!("Making HTTP request to 'stratus_appendEntries'");
 
Suggestion importance[1-10]: 6

Why: While the suggestion to enhance logging can improve traceability and debugging, it is a minor improvement and not crucial for the functionality of the code.

6

@renancloudwalk renancloudwalk merged commit 9f6acea into main May 21, 2024
26 checks passed
@renancloudwalk renancloudwalk deleted the append-entries-endpoint branch May 21, 2024 22:14
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.

2 participants