From f08fd97e8c3e37e825e725f6db8d8357f50ecb3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 4 Sep 2020 14:52:47 +0200 Subject: [PATCH 1/6] service builder: fix todo about jsonrpc Option workaround --- client/service/src/builder.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index f4046ab722ba7..003bec876f892 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -46,7 +46,7 @@ use sp_runtime::traits::{ }; use sp_api::{ProvideRuntimeApi, CallApiAt}; use sc_executor::{NativeExecutor, NativeExecutionDispatch, RuntimeInfo}; -use std::{collections::HashMap, sync::Arc}; +use std::sync::Arc; use wasm_timer::SystemTime; use sc_telemetry::{telemetry, SUBSTRATE_INFO}; use sp_transaction_pool::MaintainedTransactionPool; @@ -729,14 +729,10 @@ fn gen_handler( ); let system = system::System::new(system_info, system_rpc_tx, deny_unsafe); - let maybe_offchain_rpc = offchain_storage - .map(|storage| { + let maybe_offchain_rpc = offchain_storage.map(|storage| { let offchain = sc_rpc::offchain::Offchain::new(storage, deny_unsafe); - // FIXME: Use plain Option (don't collect into HashMap) when we upgrade to jsonrpc 14.1 - // https://github.com/paritytech/jsonrpc/commit/20485387ed06a48f1a70bf4d609a7cde6cf0accf - let delegate = offchain::OffchainApi::to_delegate(offchain); - delegate.into_iter().collect::>() - }).unwrap_or_default(); + offchain::OffchainApi::to_delegate(offchain) + }); sc_rpc_server::rpc_handler(( state::StateApi::to_delegate(state), From 06db8a4010868a0bae6f161bd34a358b28cb3a19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 4 Sep 2020 17:02:18 +0200 Subject: [PATCH 2/6] grandpa-rpc: only share executor instead of sub manager --- Cargo.lock | 2 -- bin/node/cli/Cargo.toml | 1 - bin/node/cli/src/service.rs | 2 +- bin/node/rpc/Cargo.toml | 1 - bin/node/rpc/src/lib.rs | 6 +++--- client/finality-grandpa/rpc/src/lib.rs | 15 ++++++++++----- client/service/src/builder.rs | 25 +++++++++++++++++-------- 7 files changed, 31 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9f29feece6d86..9ceda0add4394 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3747,7 +3747,6 @@ dependencies = [ "futures 0.3.5", "hex-literal", "jsonrpc-core", - "jsonrpc-pubsub", "log", "nix", "node-executor", @@ -3881,7 +3880,6 @@ name = "node-rpc" version = "2.0.0-rc6" dependencies = [ "jsonrpc-core", - "jsonrpc-pubsub", "node-primitives", "node-runtime", "pallet-contracts-rpc", diff --git a/bin/node/cli/Cargo.toml b/bin/node/cli/Cargo.toml index 92f223427a710..60a30f4e5cf64 100644 --- a/bin/node/cli/Cargo.toml +++ b/bin/node/cli/Cargo.toml @@ -39,7 +39,6 @@ serde = { version = "1.0.102", features = ["derive"] } futures = { version = "0.3.1", features = ["compat"] } hex-literal = "0.3.1" jsonrpc-core = "14.2.0" -jsonrpc-pubsub = "14.2.0" log = "0.4.8" rand = "0.7.2" structopt = { version = "0.3.8", optional = true } diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index d91696ab7d6bc..85f069a0c13fc 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -51,7 +51,7 @@ pub fn new_partial(config: &Configuration) -> Result node_rpc::IoHandler, ( sc_consensus_babe::BabeBlockImport, diff --git a/bin/node/rpc/Cargo.toml b/bin/node/rpc/Cargo.toml index 9ed8c22fbe320..e9abaef35f655 100644 --- a/bin/node/rpc/Cargo.toml +++ b/bin/node/rpc/Cargo.toml @@ -12,7 +12,6 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] jsonrpc-core = "14.2.0" -jsonrpc-pubsub = "14.2.0" node-primitives = { version = "2.0.0-rc6", path = "../primitives" } node-runtime = { version = "2.0.0-rc6", path = "../runtime" } pallet-contracts-rpc = { version = "0.8.0-rc6", path = "../../../frame/contracts/rpc/" } diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index a20fb03ebe1e6..aded77fbf96ec 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -32,7 +32,6 @@ use std::sync::Arc; -use jsonrpc_pubsub::manager::SubscriptionManager; use node_primitives::{Block, BlockNumber, AccountId, Index, Balance, Hash}; use sc_consensus_babe::{Config, Epoch}; use sc_consensus_babe_rpc::BabeRpcHandler; @@ -46,6 +45,7 @@ use sp_block_builder::BlockBuilder; use sp_blockchain::{Error as BlockChainError, HeaderMetadata, HeaderBackend}; use sp_consensus::SelectChain; use sp_consensus_babe::BabeApi; +use sc_rpc::SubscriptionTaskExecutor; use sp_transaction_pool::TransactionPool; /// Light client extra dependencies. @@ -78,8 +78,8 @@ pub struct GrandpaDeps { pub shared_authority_set: SharedAuthoritySet, /// Receives notifications about justification events from Grandpa. pub justification_stream: GrandpaJustificationStream, - /// Subscription manager to keep track of pubsub subscribers. - pub subscriptions: SubscriptionManager, + /// Executor to drive the subscription manager in the Grandpa RPC handler. + pub sub_task_executor: SubscriptionTaskExecutor, } /// Full client dependencies. diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 5606da42d5947..fedd7220d3115 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -19,6 +19,7 @@ //! RPC API for GRANDPA. #![warn(missing_docs)] +use std::sync::Arc; use futures::{FutureExt, TryFutureExt, TryStreamExt, StreamExt}; use log::warn; use jsonrpc_derive::rpc; @@ -27,6 +28,7 @@ use jsonrpc_core::futures::{ sink::Sink as Sink01, stream::Stream as Stream01, future::Future as Future01, + future::Executor as Executor01, }; mod error; @@ -92,12 +94,16 @@ pub struct GrandpaRpcHandler { impl GrandpaRpcHandler { /// Creates a new GrandpaRpcHandler instance. - pub fn new( + pub fn new( authority_set: AuthoritySet, voter_state: VoterState, justification_stream: GrandpaJustificationStream, - manager: SubscriptionManager, - ) -> Self { + executor: E, + ) -> Self + where + E: Executor01 + Send>> + Send + Sync + 'static, + { + let manager = SubscriptionManager::new(Arc::new(executor)); Self { authority_set, voter_state, @@ -232,13 +238,12 @@ mod tests { VoterState: ReportVoterState + Send + Sync + 'static, { let (justification_sender, justification_stream) = GrandpaJustificationStream::channel(); - let manager = SubscriptionManager::new(Arc::new(sc_rpc::testing::TaskExecutor)); let handler = GrandpaRpcHandler::new( TestAuthoritySet, voter_state, justification_stream, - manager, + sc_rpc::testing::TaskExecutor, ); let mut io = jsonrpc_core::MetaIoHandler::default(); diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 003bec876f892..d9e9c3e294577 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -73,17 +73,23 @@ pub trait RpcExtensionBuilder { /// Returns an instance of the RPC extension for a particular `DenyUnsafe` /// value, e.g. the RPC extension might not expose some unsafe methods. - fn build(&self, deny: sc_rpc::DenyUnsafe, subscriptions: SubscriptionManager) -> Self::Output; + fn build(&self, + deny: sc_rpc::DenyUnsafe, + sub_task_executor: sc_rpc::SubscriptionTaskExecutor + ) -> Self::Output; } impl RpcExtensionBuilder for F where - F: Fn(sc_rpc::DenyUnsafe, SubscriptionManager) -> R, + F: Fn(sc_rpc::DenyUnsafe, sc_rpc::SubscriptionTaskExecutor) -> R, R: sc_rpc::RpcExtension, { type Output = R; - fn build(&self, deny: sc_rpc::DenyUnsafe, subscriptions: SubscriptionManager) -> Self::Output { - (*self)(deny, subscriptions) + fn build(&self, + deny: sc_rpc::DenyUnsafe, + sub_task_executor: sc_rpc::SubscriptionTaskExecutor + ) -> Self::Output { + (*self)(deny, sub_task_executor) } } @@ -97,7 +103,10 @@ impl RpcExtensionBuilder for NoopRpcExtensionBuilder where { type Output = R; - fn build(&self, _deny: sc_rpc::DenyUnsafe, _subscriptions: SubscriptionManager) -> Self::Output { + fn build(&self, + _deny: sc_rpc::DenyUnsafe, + _sub_task_executor: sc_rpc::SubscriptionTaskExecutor + ) -> Self::Output { self.0.clone() } } @@ -694,7 +703,7 @@ fn gen_handler( }; let task_executor = sc_rpc::SubscriptionTaskExecutor::new(spawn_handle); - let subscriptions = SubscriptionManager::new(Arc::new(task_executor)); + let subscriptions = SubscriptionManager::new(Arc::new(task_executor.clone())); let (chain, state, child_state) = if let (Some(remote_blockchain), Some(on_demand)) = (remote_blockchain, on_demand) { @@ -723,7 +732,7 @@ fn gen_handler( let author = sc_rpc::author::Author::new( client, transaction_pool, - subscriptions.clone(), + subscriptions, keystore, deny_unsafe, ); @@ -741,7 +750,7 @@ fn gen_handler( maybe_offchain_rpc, author::AuthorApi::to_delegate(author), system::SystemApi::to_delegate(system), - rpc_extensions_builder.build(deny_unsafe, subscriptions), + rpc_extensions_builder.build(deny_unsafe, task_executor), )) } From d1605b8d4fb9a47d65b1274c24f9629945b1bd64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Mon, 7 Sep 2020 14:49:44 +0200 Subject: [PATCH 3/6] grandpa-rpc: fix compilation --- bin/node/cli/src/service.rs | 4 ++-- bin/node/rpc/src/lib.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 85f069a0c13fc..9609ec40c623f 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -119,7 +119,7 @@ pub fn new_partial(config: &Configuration) -> Result Result( shared_voter_state, shared_authority_set, justification_stream, - subscriptions, + sub_task_executor, } = grandpa; io.extend_with( @@ -172,7 +172,7 @@ pub fn create_full( shared_authority_set, shared_voter_state, justification_stream, - subscriptions, + sub_task_executor, ) ) ); From e357708172fc2a19ac17901c86a9e2a1202c737d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 8 Sep 2020 10:42:54 +0200 Subject: [PATCH 4/6] grandpa-rpc: rename to subscription_executor --- bin/node/cli/src/service.rs | 4 ++-- bin/node/rpc/src/lib.rs | 6 +++--- client/service/src/builder.rs | 8 ++++---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 9609ec40c623f..adfe83ed04857 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -119,7 +119,7 @@ pub fn new_partial(config: &Configuration) -> Result Result, /// Executor to drive the subscription manager in the Grandpa RPC handler. - pub sub_task_executor: SubscriptionTaskExecutor, + pub subscription_executor: SubscriptionTaskExecutor, } /// Full client dependencies. @@ -139,7 +139,7 @@ pub fn create_full( shared_voter_state, shared_authority_set, justification_stream, - sub_task_executor, + subscription_executor, } = grandpa; io.extend_with( @@ -172,7 +172,7 @@ pub fn create_full( shared_authority_set, shared_voter_state, justification_stream, - sub_task_executor, + subscription_executor, ) ) ); diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index d9e9c3e294577..23227c19c666f 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -75,7 +75,7 @@ pub trait RpcExtensionBuilder { /// value, e.g. the RPC extension might not expose some unsafe methods. fn build(&self, deny: sc_rpc::DenyUnsafe, - sub_task_executor: sc_rpc::SubscriptionTaskExecutor + subscription_executor: sc_rpc::SubscriptionTaskExecutor ) -> Self::Output; } @@ -87,9 +87,9 @@ impl RpcExtensionBuilder for F where fn build(&self, deny: sc_rpc::DenyUnsafe, - sub_task_executor: sc_rpc::SubscriptionTaskExecutor + subscription_executor: sc_rpc::SubscriptionTaskExecutor ) -> Self::Output { - (*self)(deny, sub_task_executor) + (*self)(deny, subscription_executor) } } @@ -105,7 +105,7 @@ impl RpcExtensionBuilder for NoopRpcExtensionBuilder where fn build(&self, _deny: sc_rpc::DenyUnsafe, - _sub_task_executor: sc_rpc::SubscriptionTaskExecutor + _subscription_executor: sc_rpc::SubscriptionTaskExecutor ) -> Self::Output { self.0.clone() } From 63e10b096d1618adcb99f17fb82b017322f0a09f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 8 Sep 2020 10:56:16 +0200 Subject: [PATCH 5/6] node/cli: remove another unused jsonrpc dependency --- Cargo.lock | 1 - bin/node/cli/Cargo.toml | 1 - 2 files changed, 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9ceda0add4394..89422a0bb1502 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3746,7 +3746,6 @@ dependencies = [ "frame-system", "futures 0.3.5", "hex-literal", - "jsonrpc-core", "log", "nix", "node-executor", diff --git a/bin/node/cli/Cargo.toml b/bin/node/cli/Cargo.toml index 60a30f4e5cf64..fdc63f09555bb 100644 --- a/bin/node/cli/Cargo.toml +++ b/bin/node/cli/Cargo.toml @@ -38,7 +38,6 @@ codec = { package = "parity-scale-codec", version = "1.3.4" } serde = { version = "1.0.102", features = ["derive"] } futures = { version = "0.3.1", features = ["compat"] } hex-literal = "0.3.1" -jsonrpc-core = "14.2.0" log = "0.4.8" rand = "0.7.2" structopt = { version = "0.3.8", optional = true } From 28ce3dc95a57e02c1cad4e04e6bb533cd6b06fd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= <123550+andresilva@users.noreply.github.com> Date: Mon, 14 Sep 2020 11:31:27 +0100 Subject: [PATCH 6/6] grandpa: apply style fixes from code review --- bin/node/cli/src/service.rs | 2 +- client/service/src/builder.rs | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index adfe83ed04857..b972efa813d2f 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -51,7 +51,7 @@ pub fn new_partial(config: &Configuration) -> Result node_rpc::IoHandler, ( sc_consensus_babe::BabeBlockImport, diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 23227c19c666f..5a1737af3724a 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -73,9 +73,10 @@ pub trait RpcExtensionBuilder { /// Returns an instance of the RPC extension for a particular `DenyUnsafe` /// value, e.g. the RPC extension might not expose some unsafe methods. - fn build(&self, + fn build( + &self, deny: sc_rpc::DenyUnsafe, - subscription_executor: sc_rpc::SubscriptionTaskExecutor + subscription_executor: sc_rpc::SubscriptionTaskExecutor, ) -> Self::Output; } @@ -85,9 +86,10 @@ impl RpcExtensionBuilder for F where { type Output = R; - fn build(&self, + fn build( + &self, deny: sc_rpc::DenyUnsafe, - subscription_executor: sc_rpc::SubscriptionTaskExecutor + subscription_executor: sc_rpc::SubscriptionTaskExecutor, ) -> Self::Output { (*self)(deny, subscription_executor) } @@ -103,9 +105,10 @@ impl RpcExtensionBuilder for NoopRpcExtensionBuilder where { type Output = R; - fn build(&self, + fn build( + &self, _deny: sc_rpc::DenyUnsafe, - _subscription_executor: sc_rpc::SubscriptionTaskExecutor + _subscription_executor: sc_rpc::SubscriptionTaskExecutor, ) -> Self::Output { self.0.clone() }