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(jsonrpc): move nearcore type conversions to server #6902

Merged
merged 11 commits into from
May 31, 2022

Conversation

miraclx
Copy link
Contributor

@miraclx miraclx commented May 27, 2022

Tracking issue: #6850

Move all conversions between near-primitives and near-jsonrpc-primitives to the server side. Freeing up a bunch of dependencies on near-jsonrpc-primitives.

@miraclx miraclx requested review from frol, matklad and khorolets May 27, 2022 18:43
@miraclx miraclx requested a review from a team as a code owner May 27, 2022 18:43
@matklad
Copy link
Contributor

matklad commented May 30, 2022

I think this can be handled in a simpler way, without adding our own conversion trait/macro:

diff --git a/chain/jsonrpc/src/errors.rs b/chain/jsonrpc/src/errors.rs
index f71d3217b..9082dc5cd 100644
--- a/chain/jsonrpc/src/errors.rs
+++ b/chain/jsonrpc/src/errors.rs
@@ -11,6 +11,10 @@ macro_rules! _rpc_try {
 
 pub(crate) use _rpc_try as rpc_try;
 
+pub(crate) struct InternalServerError {
+    pub(crate) message: String,
+}
+
 pub trait RpcFrom<T> {
     fn rpc_from(_: T) -> Self;
 }
@@ -36,9 +40,9 @@ impl RpcFrom<actix::MailboxError> for ServerError {
     }
 }
 
-impl RpcFrom<actix::MailboxError> for near_jsonrpc_primitives::types::blocks::RpcBlockError {
-    fn rpc_from(error: actix::MailboxError) -> Self {
-        Self::InternalError { error_message: error.to_string() }
+impl From<InternalServerError> for near_jsonrpc_primitives::types::blocks::RpcBlockError {
+    fn from(error: InternalServerError) -> Self {
+        Self::InternalError { error_message: error.message }
     }
 }
 
diff --git a/chain/jsonrpc/src/lib.rs b/chain/jsonrpc/src/lib.rs
index e76d9cbbc..7dac68f5d 100644
--- a/chain/jsonrpc/src/lib.rs
+++ b/chain/jsonrpc/src/lib.rs
@@ -5,6 +5,7 @@ use std::time::{Duration, Instant};
 use actix::Addr;
 use actix_cors::Cors;
 use actix_web::{get, http, middleware, web, App, Error as HttpError, HttpResponse, HttpServer};
+use errors::InternalServerError;
 use futures::Future;
 use futures::FutureExt;
 use prometheus;
@@ -984,12 +985,7 @@ impl JsonRpcHandler {
         near_jsonrpc_primitives::types::blocks::RpcBlockResponse,
         near_jsonrpc_primitives::types::blocks::RpcBlockError,
     > {
-        let block_view = rpc_try! {
-            self
-                .view_client_addr
-                .send(GetBlock(request_data.block_reference))
-                .await
-        }?;
+        let block_view = self.message_view_client(GetBlock(request_data.block_reference)).await??;
         Ok(near_jsonrpc_primitives::types::blocks::RpcBlockResponse { block_view })
     }
 
@@ -1188,6 +1184,18 @@ impl JsonRpcHandler {
         }?;
         Ok(validators)
     }
+
+    async fn message_view_client<M>(&self, msg: M) -> Result<M::Result, InternalServerError>
+    where
+        M: actix::Message + Send + 'static,
+        M::Result: Send,
+        ViewClientActor: actix::Handler<M>,
+    {
+        self.view_client_addr
+            .send(msg)
+            .await
+            .map_err(|it| InternalServerError { message: it.to_string() })
+    }
 }
 
 #[cfg(feature = "sandbox")]

There's a bit of logical copy-paste with handling actix messages. So we can extract that bit and do conversion once, rather than doing it on every call-site.

chain/jsonrpc/src/lib.rs Outdated Show resolved Hide resolved
@miraclx miraclx changed the title chore(jsonrpc): move actix error conversions to server chore(jsonrpc): move nearcore type conversions to server May 30, 2022
@miraclx miraclx force-pushed the decouple/jsonrpc/remove-actix branch from 671f70f to f516a37 Compare May 30, 2022 20:06
Comment on lines +23 to +25
near-chain-configs = { path = "../../core/chain-configs" }
near-rpc-error-macro = { path = "../../tools/rpctypegen/macro" }
near-client-primitives = { path = "../client-primitives" }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the initial read, I thought we introduced new cruel dependencies chain-configs and client-primitives, but in fact, they just got shuffled a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more PRs and they'll be gone for good.

@near-bulldozer near-bulldozer bot merged commit ac19a77 into master May 31, 2022
@near-bulldozer near-bulldozer bot deleted the decouple/jsonrpc/remove-actix branch May 31, 2022 10:05
near-bulldozer bot pushed a commit that referenced this pull request May 31, 2022
Tagged: https://github.com/near/nearcore/releases/tag/crates-0.14.0

Notable changes:

  - #6844
  - #6853
  - #6902
  - #6917
  - #6916

Check here for a full changelog: crates-0.13.0...crates-0.14.0

Tested `cargo package` on each public crate locally, they all compile independently of the workspace. We should be good to go.
near-bulldozer bot pushed a commit that referenced this pull request May 31, 2022
Tracking issue: #6850

Requires: #6902

Splits up the `parser` and `errors` module into finer and more specialized modules, consolidating both parsing and type conversion logic.
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.

3 participants