fix: implement a temporary proxy layer for get requests

[carneiro-cw]

User description

Temporary until paritytech/jsonrpsee#1512 is merged and released. If the changes in the jsonrpsee repo are not accepted we'll make these changes permanent.

---

PR Type

Enhancement, Bug fix

---

Description

• Implement temporary proxy layer for GET requests

• Update jsonrpsee dependency and add related packages

• Modify RPC middleware and server configurations

• Adjust error handling in RPC responses

---

Changes walkthrough 📝

	Relevant files
Enhancement	mod.rs Add ErrorCode to public exports                                                    src/eth/primitives/mod.rs Added ErrorCode to the public exports +1/-0      mod.rs Add proxy_get_request module                                                          src/eth/rpc/mod.rs Added new module proxy_get_request +1/-0      proxy_get_request.rs Implement temporary proxy for GET requests                              src/eth/rpc/proxy_get_request.rs Implemented ProxyGetRequestTemp and ProxyGetRequestTempLayer Added functionality to proxy GET requests to RPC method calls Included error handling and request/response modifications +214/-0  rpc_server.rs Replace ProxyGetRequestLayer with temporary version            src/eth/rpc/rpc_server.rs Replaced ProxyGetRequestLayer with ProxyGetRequestTempLayer Updated import statements +5/-5
Bug fix	rpc_middleware.rs Update error handling in RPC responses                                      src/eth/rpc/rpc_middleware.rs Modified error handling in RPC responses Replaced hardcoded error label with dynamic error code representation +2/-1
Miscellaneous	mod.rs Refactor import statements in test module                                src/eth/storage/permanent/rocks/types/mod.rs Adjusted import statements in test module +1/-1
Tests	e2e-json-rpc.test.ts Add tests for GET requests to health endpoint                        e2e/test/automine/e2e-json-rpc.test.ts Added tests for GET requests to health endpoint Included checks for request failures and successes +6/-0
Dependencies	Cargo.toml Update jsonrpsee and add related dependencies                        Cargo.toml Updated jsonrpsee to version 0.24.7 Added new dependencies: http-body, http-body-util, bytes +4/-1

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The error handling in the proxy layer might need improvement. The current implementation uses expect in some places, which could lead to panics in production. Consider using more robust error handling techniques. #[allow(clippy::expect_used)] fn layer(&self, inner: S) -> Self::Service { ProxyGetRequestTemp::new(inner, &self.path, &self.method).expect("Path already validated in ProxyGetRequestLayer; qed") } Metrics Labeling The changes to the metrics labeling logic might affect the accuracy of the metrics. Ensure that the new error code representation is consistent with the existing metrics system and provides the necessary information for monitoring and debugging. let rpc_result = match response_result.get("result") { Some(result) => if_else!(result.is_null(), metrics::LABEL_MISSING, metrics::LABEL_PRESENT), None => StratusError::str_repr_from_err_code(error_code), }; Temporary Solution The use of ProxyGetRequestTempLayer is mentioned as a temporary solution. Ensure there's a plan to replace this with a permanent solution once the related PR in the jsonrpsee repository is merged and released. .layer(ProxyGetRequestTempLayer::new("/health", "stratus_health").unwrap()) .layer(ProxyGetRequestTempLayer::new("/version", "stratus_version").unwrap()) .layer(ProxyGetRequestTempLayer::new("/config", "stratus_config").unwrap()) .layer(ProxyGetRequestTempLayer::new("/state", "stratus_state").unwrap());

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Improve error handling for JSON deserialization to provide more informative error responses Consider using a more robust error handling approach for the JSON deserialization. The current implementation returns an internal error if the deserialization fails, which might hide important error details. Instead, you could log the error and return a more specific error response. src/eth/rpc/proxy_get_request.rs [203-207] -let response = if let Ok(payload) = serde_json::from_slice::<RpcPayload>(&bytes) { - ok_response(payload.result.to_string()) -} else { - internal_error() +let response = match serde_json::from_slice::<RpcPayload>(&bytes) { + Ok(payload) => ok_response(payload.result.to_string()), + Err(e) => { + log::error!("Failed to deserialize RPC payload: {}", e); + jsonrpsee::server::http::response::error_response( + jsonrpsee::types::ErrorObject::borrowed( + jsonrpsee::types::ErrorCode::InternalError.code(), + "Failed to process response", + None, + ) + ) + } }; Suggestion importance[1-10]: 7 Why: The suggestion improves error handling and logging, which can aid in debugging and provide more informative responses to clients. This enhancement is valuable for maintainability and user experience.	7
General	Enhance health endpoint test to verify both status code and response content The test for the health endpoint is incomplete. It only checks for a 200 status code but doesn't verify the response body. Add an assertion to check the content of the response to ensure it contains the expected health information. e2e/test/automine/e2e-json-rpc.test.ts [83-84] const healthResponse = await fetch(`http://localhost:3000/health?app=test-client`); expect(healthResponse.status).eq(200); +const healthData = await healthResponse.json(); +expect(healthData).to.have.property('status').that.equals('ok'); Suggestion importance[1-10]: 6 Why: This suggestion improves the test coverage by verifying both the status code and the response content of the health endpoint. This enhancement ensures more comprehensive testing of the API's functionality.	6
	Optimize memory usage by replacing Arc with String for small string values The ProxyGetRequestTemp struct is using Arc for path and method. While this is not necessarily wrong, it might be overkill for these small string values. Consider using String instead, which would simplify the code and potentially improve performance for short strings. src/eth/rpc/proxy_get_request.rs [104-109] pub struct ProxyGetRequestTemp<S> { inner: S, - path: Arc<str>, - method: Arc<str>, + path: String, + method: String, } Suggestion importance[1-10]: 3 Why: While the suggestion might slightly optimize memory usage, the impact is likely minimal. The current implementation using Arc is not incorrect and may have been chosen for specific reasons related to concurrency or lifetime management.	3

[gabriel-aranha-cw]

Final benchmark:
Run ID: bench-624811985

Git Info:

• Branch: main
• Commit: 7ee665a
• PR: fix: implement a temporary proxy layer for get requests #1940

Configuration:

• Target Account Strategy: Default

RPS Stats: Max: 1292.00, Min: 674.00, Avg: 1173.50, StdDev: 49.38
TPS Stats: Max: 1275.00, Min: 1018.00, Avg: 1138.75, StdDev: 51.50

Plot: View Plot