From 68ff17e84c55509161d8c4cfd0d14d03cad916a3 Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Fri, 17 Sep 2021 08:26:13 +0200 Subject: [PATCH] jsonrpsee more cleanup (#9803) * more cleanup * resolve TODOs * fix some unwraps * remove type hints --- Cargo.lock | 1 + bin/node-template/node/src/rpc.rs | 19 ++-- bin/node-template/node/src/service.rs | 26 +++-- bin/node/cli/src/service.rs | 141 +++++++++++-------------- bin/node/rpc/src/lib.rs | 2 +- client/finality-grandpa/rpc/src/lib.rs | 4 +- client/rpc/src/author/mod.rs | 14 +-- client/rpc/src/chain/chain_full.rs | 4 +- client/rpc/src/chain/chain_light.rs | 6 +- client/rpc/src/chain/mod.rs | 6 +- client/rpc/src/lib.rs | 13 --- client/rpc/src/state/mod.rs | 6 +- client/rpc/src/state/state_full.rs | 51 +++++---- client/rpc/src/state/state_light.rs | 12 +-- client/service/src/builder.rs | 48 ++++----- client/service/src/lib.rs | 20 ++-- frame/contracts/rpc/Cargo.toml | 1 + frame/contracts/rpc/src/lib.rs | 24 ++--- test-utils/test-runner/src/client.rs | 9 +- 19 files changed, 193 insertions(+), 214 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index af95156e7f73a..209fcb00c375d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4866,6 +4866,7 @@ dependencies = [ name = "pallet-contracts-rpc" version = "4.0.0-dev" dependencies = [ + "anyhow", "jsonrpsee", "log", "pallet-contracts-primitives", diff --git a/bin/node-template/node/src/rpc.rs b/bin/node-template/node/src/rpc.rs index f614dca34158d..ce12f9018ea25 100644 --- a/bin/node-template/node/src/rpc.rs +++ b/bin/node-template/node/src/rpc.rs @@ -7,13 +7,15 @@ use std::sync::Arc; +use jsonrpsee::RpcModule; use node_template_runtime::{opaque::Block, AccountId, Balance, Index}; -pub use sc_rpc_api::DenyUnsafe; use sc_transaction_pool_api::TransactionPool; use sp_api::ProvideRuntimeApi; use sp_block_builder::BlockBuilder; use sp_blockchain::{Error as BlockChainError, HeaderBackend, HeaderMetadata}; +pub use sc_rpc_api::DenyUnsafe; + /// Full client dependencies. pub struct FullDeps { /// The client instance to use. @@ -25,7 +27,9 @@ pub struct FullDeps { } /// Instantiate all full RPC extensions. -pub fn create_full(deps: FullDeps) -> jsonrpsee::RpcModule<()> +pub fn create_full( + deps: FullDeps, +) -> Result, Box> where C: ProvideRuntimeApi, C: HeaderBackend + HeaderMetadata + 'static, @@ -38,18 +42,17 @@ where use pallet_transaction_payment_rpc::{TransactionPaymentApiServer, TransactionPaymentRpc}; use substrate_frame_rpc_system::{SystemApiServer, SystemRpc, SystemRpcBackendFull}; - let mut module = jsonrpsee::RpcModule::new(()); + let mut module = RpcModule::new(()); let FullDeps { client, pool, deny_unsafe } = deps; let system_rpc_backend = SystemRpcBackendFull::new(client.clone(), pool.clone(), deny_unsafe); - module.merge(SystemRpc::new(Box::new(system_rpc_backend)).into_rpc()).unwrap(); - - module.merge(TransactionPaymentRpc::new(client.clone()).into_rpc()).unwrap(); + module.merge(SystemRpc::new(Box::new(system_rpc_backend)).into_rpc())?; + module.merge(TransactionPaymentRpc::new(client.clone()).into_rpc())?; // Extend this RPC with a custom API by using the following syntax. // `YourRpcStruct` should have a reference to a client, which is needed // to call into the runtime. - // `module.merge(YourRpcTrait::into_rpc(YourRpcStruct::new(ReferenceToClient, ...))).unwrap();` + // `module.merge(YourRpcTrait::into_rpc(YourRpcStruct::new(ReferenceToClient, ...)))?;` - module + Ok(module) } diff --git a/bin/node-template/node/src/service.rs b/bin/node-template/node/src/service.rs index e08b0b4278ba9..60fea344e4f30 100644 --- a/bin/node-template/node/src/service.rs +++ b/bin/node-template/node/src/service.rs @@ -1,6 +1,5 @@ //! Service and ServiceFactory implementation. Specialized wrapper over substrate service. -use jsonrpsee::RpcModule; use node_template_runtime::{self, opaque::Block, RuntimeApi}; use sc_client_api::{ExecutorProvider, RemoteBackend}; use sc_consensus_aura::{ImportQueueParams, SlotProportion, StartAuraParams}; @@ -56,7 +55,7 @@ pub fn new_partial( ServiceError, > { if config.keystore_remote.is_some() { - return Err(ServiceError::Other(format!("Remote Keystores are not supported."))) + return Err(ServiceError::Other(format!("Remote Keystores are not supported."))); } let telemetry = config @@ -141,7 +140,6 @@ pub fn new_partial( keystore_container, select_chain, transaction_pool, - rpc_builder: Box::new(|_, _| RpcModule::new(())), other: (grandpa_block_import, grandpa_link, telemetry), }) } @@ -163,18 +161,18 @@ pub fn new_full(mut config: Configuration) -> Result mut keystore_container, select_chain, transaction_pool, - rpc_builder: _rpc_builder, other: (block_import, grandpa_link, mut telemetry), } = new_partial(&config)?; if let Some(url) = &config.keystore_remote { match remote_keystore(url) { Ok(k) => keystore_container.set_remote_keystore(k), - Err(e) => + Err(e) => { return Err(ServiceError::Other(format!( "Error hooking up remote keystore for {}: {}", url, e - ))), + ))) + } }; } @@ -212,14 +210,24 @@ pub fn new_full(mut config: Configuration) -> Result let enable_grandpa = !config.disable_grandpa; let prometheus_registry = config.prometheus_registry().cloned(); + let rpc_extensions_builder = { + let client = client.clone(); + let pool = transaction_pool.clone(); + + Box::new(move |deny_unsafe, _| { + let deps = + crate::rpc::FullDeps { client: client.clone(), pool: pool.clone(), deny_unsafe }; + crate::rpc::create_full(deps).map_err(Into::into) + }) + }; + let _rpc_handlers = sc_service::spawn_tasks(sc_service::SpawnTasksParams { network: network.clone(), client: client.clone(), keystore: keystore_container.sync_keystore(), task_manager: &mut task_manager, transaction_pool: transaction_pool.clone(), - // TODO: (dp) implement - rpc_builder: Box::new(|_, _| RpcModule::new(())), + rpc_builder: rpc_extensions_builder, on_demand: None, remote_blockchain: None, backend, @@ -451,7 +459,7 @@ pub fn new_light(mut config: Configuration) -> Result Box::new(move |deny_unsafe, _| { let deps = crate::rpc::FullDeps { client: client.clone(), pool: pool.clone(), deny_unsafe }; - crate::rpc::create_full(deps) + crate::rpc::create_full(deps).map_err(Into::into) }) }; diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 704b1dd163fac..291e002d50b9a 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -33,15 +33,6 @@ use sc_telemetry::{Telemetry, TelemetryWorker}; use sp_runtime::traits::Block as BlockT; use std::sync::Arc; -use jsonrpsee::RpcModule; -use pallet_contracts_rpc::{ContractsApiServer, ContractsRpc}; -use pallet_mmr_rpc::{MmrApiServer, MmrRpc}; -use pallet_transaction_payment_rpc::{TransactionPaymentApiServer, TransactionPaymentRpc}; -use sc_consensus_babe_rpc::{BabeApiServer, BabeRpc}; -use sc_finality_grandpa_rpc::{GrandpaApiServer, GrandpaRpc}; -use sc_sync_state_rpc::{SyncStateRpc, SyncStateRpcApiServer}; -use substrate_frame_rpc_system::{SystemApiServer, SystemRpc, SystemRpcBackendFull}; - type FullClient = sc_service::TFullClient>; type FullBackend = sc_service::TFullBackend; @@ -63,12 +54,16 @@ pub fn new_partial( sc_consensus::DefaultImportQueue, sc_transaction_pool::FullPool, ( - // Block import setup. + impl Fn( + node_rpc::DenyUnsafe, + sc_rpc::SubscriptionTaskExecutor, + ) -> Result, sc_service::Error>, ( sc_consensus_babe::BabeBlockImport, grandpa::LinkHalf, sc_consensus_babe::BabeLink, ), + grandpa::SharedVoterState, Option, ), >, @@ -155,69 +150,56 @@ pub fn new_partial( telemetry.as_ref().map(|x| x.handle()), )?; - // Grandpa stuff - let shared_authority_set = grandpa_link.shared_authority_set().clone(); - let justification_stream = grandpa_link.justification_stream().clone(); - let backend2 = backend.clone(); - // Babe stuff - let select_chain2 = select_chain.clone(); - let sync_keystore = keystore_container.sync_keystore().clone(); - let client2 = client.clone(); - let babe_link2 = babe_link.clone(); - // SyncState - let chain_spec = config.chain_spec.cloned_box(); - let shared_epoch_changes = babe_link.epoch_changes().clone(); - // System - let transaction_pool2 = transaction_pool.clone(); - let rpc_builder = Box::new(move |deny_unsafe, executor| -> RpcModule<()> { - let grandpa_rpc = GrandpaRpc::new( - executor, - shared_authority_set.clone(), - grandpa::SharedVoterState::empty(), - justification_stream, - grandpa::FinalityProofProvider::new_for_service( - backend2, - Some(shared_authority_set.clone()), - ), - ) - .into_rpc(); - - let babe_rpc = BabeRpc::new( - client2.clone(), - babe_link.epoch_changes().clone(), - sync_keystore, - babe_link.config().clone(), - select_chain2, - deny_unsafe, - ) - .into_rpc(); - let sync_state_rpc = SyncStateRpc::new( - chain_spec, - client2.clone(), - shared_authority_set.clone(), - shared_epoch_changes, - deny_unsafe, - ) - .expect("TODO: error handling") - .into_rpc(); - let transaction_payment_rpc = TransactionPaymentRpc::new(client2.clone()).into_rpc(); - let system_rpc_backend = - SystemRpcBackendFull::new(client2.clone(), transaction_pool2.clone(), deny_unsafe); - let system_rpc = SystemRpc::new(Box::new(system_rpc_backend)).into_rpc(); - let mmr_rpc = MmrRpc::new(client2.clone()).into_rpc(); - let contracts_rpc = ContractsRpc::new(client2.clone()).into_rpc(); - let mut module = RpcModule::new(()); - module.merge(grandpa_rpc).expect("TODO: error handling"); - module.merge(babe_rpc).expect("TODO: error handling"); - module.merge(sync_state_rpc).expect("TODO: error handling"); - module.merge(transaction_payment_rpc).expect("TODO: error handling"); - module.merge(system_rpc).expect("TODO: error handling"); - module.merge(mmr_rpc).expect("TODO: error handling"); - module.merge(contracts_rpc).expect("TODO: error handling"); - module - }); + let import_setup = (block_import, grandpa_link, babe_link); + + let (rpc_extensions_builder, rpc_setup) = { + let (_, grandpa_link, babe_link) = &import_setup; + + let justification_stream = grandpa_link.justification_stream(); + let shared_authority_set = grandpa_link.shared_authority_set().clone(); + let shared_voter_state = grandpa::SharedVoterState::empty(); + let rpc_setup = shared_voter_state.clone(); - let import_setup = (block_import, grandpa_link, babe_link2); + let finality_proof_provider = grandpa::FinalityProofProvider::new_for_service( + backend.clone(), + Some(shared_authority_set.clone()), + ); + + let babe_config = babe_link.config().clone(); + let shared_epoch_changes = babe_link.epoch_changes().clone(); + + let client = client.clone(); + let pool = transaction_pool.clone(); + let select_chain = select_chain.clone(); + let keystore = keystore_container.sync_keystore(); + let chain_spec = config.chain_spec.cloned_box(); + + let rpc_extensions_builder = move |deny_unsafe, subscription_executor| { + let deps = node_rpc::FullDeps { + client: client.clone(), + pool: pool.clone(), + select_chain: select_chain.clone(), + chain_spec: chain_spec.cloned_box(), + deny_unsafe, + babe: node_rpc::BabeDeps { + babe_config: babe_config.clone(), + shared_epoch_changes: shared_epoch_changes.clone(), + keystore: keystore.clone(), + }, + grandpa: node_rpc::GrandpaDeps { + shared_voter_state: shared_voter_state.clone(), + shared_authority_set: shared_authority_set.clone(), + justification_stream: justification_stream.clone(), + subscription_executor, + finality_provider: finality_proof_provider.clone(), + }, + }; + + node_rpc::create_full(deps).map_err(Into::into) + }; + + (rpc_extensions_builder, rpc_setup) + }; Ok(sc_service::PartialComponents { client, @@ -227,8 +209,7 @@ pub fn new_partial( select_chain, import_queue, transaction_pool, - rpc_builder, - other: (import_setup, telemetry), + other: (rpc_extensions_builder, import_setup, rpc_setup, telemetry), }) } @@ -255,10 +236,10 @@ pub fn new_full_base( keystore_container, select_chain, transaction_pool, - rpc_builder, - other: (import_setup, mut telemetry), + other: (rpc_extensions_builder, import_setup, rpc_setup, mut telemetry), } = new_partial(&config)?; + let shared_voter_state = rpc_setup; let auth_disc_publish_non_global_ips = config.network.allow_non_globals_in_dht; config.network.extra_sets.push(grandpa::grandpa_peers_set_config()); @@ -302,7 +283,7 @@ pub fn new_full_base( client: client.clone(), keystore: keystore_container.sync_keystore(), network: network.clone(), - rpc_builder: Box::new(rpc_builder), + rpc_builder: Box::new(rpc_extensions_builder), transaction_pool: transaction_pool.clone(), task_manager: &mut task_manager, on_demand: None, @@ -434,7 +415,7 @@ pub fn new_full_base( telemetry: telemetry.as_ref().map(|x| x.handle()), voting_rule: grandpa::VotingRulesBuilder::default().build(), prometheus_registry, - shared_voter_state: grandpa::SharedVoterState::empty(), + shared_voter_state, }; // the GRANDPA voter task is considered infallible, i.e. @@ -601,7 +582,7 @@ pub fn new_light_base( pool: transaction_pool.clone(), }; - let rpc_builder = Box::new(move |_, _| -> RpcModule<()> { node_rpc::create_light(light_deps) }); + let rpc_builder = Box::new(move |_, _| Ok(node_rpc::create_light(light_deps))); sc_service::spawn_tasks(sc_service::SpawnTasksParams { on_demand: Some(on_demand), @@ -761,7 +742,7 @@ mod tests { sc_consensus_babe::authorship::claim_slot(slot.into(), &epoch, &keystore) .map(|(digest, _)| digest) { - break (babe_pre_digest, epoch_descriptor) + break (babe_pre_digest, epoch_descriptor); } slot += 1; diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index 81303fb4c1e71..5d4ffb564acf2 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -171,7 +171,7 @@ where )?; io.merge( GrandpaRpc::new( - Arc::new(subscription_executor), + subscription_executor, shared_authority_set.clone(), shared_voter_state, justification_stream, diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index e942bcca4c4a1..07cfc6a1b0fbd 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -67,7 +67,7 @@ pub trait GrandpaApi { /// Provides RPC methods for interacting with GRANDPA. pub struct GrandpaRpc { - executor: Arc, + executor: SubscriptionTaskExecutor, authority_set: AuthoritySet, voter_state: VoterState, justification_stream: GrandpaJustificationStream, @@ -78,7 +78,7 @@ impl { /// Prepare a new [`GrandpaApi`] pub fn new( - executor: Arc, + executor: SubscriptionTaskExecutor, authority_set: AuthoritySet, voter_state: VoterState, justification_stream: GrandpaJustificationStream, diff --git a/client/rpc/src/author/mod.rs b/client/rpc/src/author/mod.rs index 67b6bdd4c5ed6..80f4711cea50c 100644 --- a/client/rpc/src/author/mod.rs +++ b/client/rpc/src/author/mod.rs @@ -58,7 +58,7 @@ pub struct Author { /// Whether to deny unsafe calls deny_unsafe: DenyUnsafe, /// Executor to spawn subscriptions. - executor: Arc, + executor: SubscriptionTaskExecutor, } impl Author { @@ -68,7 +68,7 @@ impl Author { pool: Arc

, keystore: SyncCryptoStorePtr, deny_unsafe: DenyUnsafe, - executor: Arc, + executor: SubscriptionTaskExecutor, ) -> Self { Author { client, pool, keystore, deny_unsafe, executor } } @@ -156,7 +156,7 @@ where hash::ExtrinsicOrHash::Extrinsic(bytes) => { let xt = Decode::decode(&mut &bytes[..])?; Ok(self.pool.hash_of(&xt)) - }, + } }) .collect::>>()?; @@ -174,8 +174,8 @@ where Ok(dxt) => dxt, Err(e) => { log::error!("[watch_extrinsic sub] failed to decode extrinsic: {:?}", e); - return Err(JsonRpseeError::to_call_error(e)) - }, + return Err(JsonRpseeError::to_call_error(e)); + } }; let executor = self.executor.clone(); @@ -191,8 +191,8 @@ where "txpool subscription failed: {:?}; subscription useless", e )); - return - }, + return; + } }; stream diff --git a/client/rpc/src/chain/chain_full.rs b/client/rpc/src/chain/chain_full.rs index 656141cc30347..b173d785bb187 100644 --- a/client/rpc/src/chain/chain_full.rs +++ b/client/rpc/src/chain/chain_full.rs @@ -37,12 +37,12 @@ pub struct FullChain { /// phantom member to pin the block type _phantom: PhantomData, /// Subscription executor. - executor: Arc, + executor: SubscriptionTaskExecutor, } impl FullChain { /// Create new Chain API RPC handler. - pub fn new(client: Arc, executor: Arc) -> Self { + pub fn new(client: Arc, executor: SubscriptionTaskExecutor) -> Self { Self { client, executor, _phantom: PhantomData } } } diff --git a/client/rpc/src/chain/chain_light.rs b/client/rpc/src/chain/chain_light.rs index be6da9417a678..654d05a9ca30d 100644 --- a/client/rpc/src/chain/chain_light.rs +++ b/client/rpc/src/chain/chain_light.rs @@ -43,7 +43,7 @@ pub struct LightChain { /// Remote fetcher reference. fetcher: Arc, /// Subscription executor. - executor: Arc, + executor: SubscriptionTaskExecutor, } impl> LightChain { @@ -52,7 +52,7 @@ impl> LightChain { client: Arc, remote_blockchain: Arc>, fetcher: Arc, - executor: Arc, + executor: SubscriptionTaskExecutor, ) -> Self { Self { client, executor, remote_blockchain, fetcher } } @@ -94,7 +94,7 @@ where let body = fetcher.remote_body(req_body).await.map_err(client_err)?; Ok(Some(SignedBlock { block: Block::new(header, body), justifications: None })) - }, + } None => Ok(None), } } diff --git a/client/rpc/src/chain/mod.rs b/client/rpc/src/chain/mod.rs index 181d077b3668a..7753171bd6d82 100644 --- a/client/rpc/src/chain/mod.rs +++ b/client/rpc/src/chain/mod.rs @@ -96,7 +96,7 @@ where .header(BlockId::number(block_num)) .map_err(client_err)? .map(|h| h.hash())) - }, + } } } @@ -118,7 +118,7 @@ where /// Create new state API that works on full node. pub fn new_full( client: Arc, - executor: Arc, + executor: SubscriptionTaskExecutor, ) -> Chain where Block: BlockT + 'static, @@ -131,7 +131,7 @@ where /// Create new state API that works on light node. pub fn new_light>( client: Arc, - executor: Arc, + executor: SubscriptionTaskExecutor, remote_blockchain: Arc>, fetcher: Arc, ) -> Chain diff --git a/client/rpc/src/lib.rs b/client/rpc/src/lib.rs index 2d0666714e131..7dca345aa934d 100644 --- a/client/rpc/src/lib.rs +++ b/client/rpc/src/lib.rs @@ -51,16 +51,3 @@ impl SubscriptionTaskExecutor { let _ = self.0.spawn("substrate-rpc-subscriber", fut); } } - -/// Helper macro to bail early in async context when you want to -/// return `Box::pin(future::err(e))` once an error occurs. -/// Because `Try` is not implemented for it. -#[macro_export] -macro_rules! unwrap_or_fut_err { - ( $e:expr ) => { - match $e { - Ok(x) => x, - Err(e) => return Box::pin(future::err(e.into())), - } - }; -} diff --git a/client/rpc/src/state/mod.rs b/client/rpc/src/state/mod.rs index 7bd5c78fb6cdb..0f362592bb0d9 100644 --- a/client/rpc/src/state/mod.rs +++ b/client/rpc/src/state/mod.rs @@ -171,7 +171,7 @@ where /// Create new state API that works on full node. pub fn new_full( client: Arc, - executor: Arc, + executor: SubscriptionTaskExecutor, deny_unsafe: DenyUnsafe, rpc_max_payload: Option, ) -> (StateApi, ChildState) @@ -205,7 +205,7 @@ where /// Create new state API that works on light node. pub fn new_light>( client: Arc, - executor: Arc, + executor: SubscriptionTaskExecutor, remote_blockchain: Arc>, fetcher: Arc, deny_unsafe: DenyUnsafe, @@ -296,7 +296,7 @@ where return Err(JsonRpseeError::to_call_error(Error::InvalidCount { value: count, max: STORAGE_KEYS_PAGED_MAX_COUNT, - })) + })); } self.backend .storage_keys_paged(block, prefix, count, start_key) diff --git a/client/rpc/src/state/state_full.rs b/client/rpc/src/state/state_full.rs index 56d92a1d0b8e5..654a08eaf3597 100644 --- a/client/rpc/src/state/state_full.rs +++ b/client/rpc/src/state/state_full.rs @@ -73,7 +73,7 @@ struct QueryStorageRange { /// State API backend for full nodes. pub struct FullState { client: Arc, - executor: Arc, + executor: SubscriptionTaskExecutor, _phantom: PhantomData<(BE, Block)>, rpc_max_payload: Option, } @@ -90,7 +90,7 @@ where /// Create new state API backend for full nodes. pub fn new( client: Arc, - executor: Arc, + executor: SubscriptionTaskExecutor, rpc_max_payload: Option, ) -> Self { Self { client, executor, _phantom: PhantomData, rpc_max_payload } @@ -123,7 +123,7 @@ where &from_meta, &to_meta, "from number > to number".to_owned(), - )) + )); } // check if we can get from `to` to `from` by going through parent_hashes. @@ -144,7 +144,7 @@ where &from_meta, &to_meta, "from and to are on different forks".to_owned(), - )) + )); } hashes.reverse(); hashes @@ -226,7 +226,7 @@ where let key_changes = self.client.key_changes(begin, end, None, key).map_err(client_err)?; for (block, _) in key_changes.into_iter().rev() { if last_block == Some(block) { - continue + continue; } let block_hash = @@ -234,7 +234,7 @@ where let id = BlockId::Hash(block_hash); let value_at_block = self.client.storage(&id, key).map_err(client_err)?; if last_value == value_at_block { - continue + continue; } changes_map @@ -358,7 +358,7 @@ where match self.client.storage(&BlockId::Hash(block), &key) { Ok(Some(d)) => return Ok(Some(d.0.len() as u64)), Err(e) => return Err(client_err(e)), - Ok(None) => {}, + Ok(None) => {} } self.client @@ -466,17 +466,18 @@ where .filter_map(move |n| { let version = client.runtime_version_at(&BlockId::hash(n.hash)); match version { - Ok(v) => + Ok(v) => { if previous_version != v { previous_version = v.clone(); future::ready(Some(v)) } else { future::ready(None) - }, + } + } Err(e) => { log::error!("Could not fetch current runtime version. Error={:?}", e); future::ready(None) - }, + } } }) .take_while(|version| { @@ -614,8 +615,9 @@ where self.block_or_best(block) .and_then(|block| { let child_info = match ChildType::from_prefixed_key(&storage_key) { - Some((ChildType::ParentKeyId, storage_key)) => - ChildInfo::new_default(storage_key), + Some((ChildType::ParentKeyId, storage_key)) => { + ChildInfo::new_default(storage_key) + } None => return Err(sp_blockchain::Error::InvalidChildStorageKey), }; self.client @@ -639,8 +641,9 @@ where self.block_or_best(block) .and_then(|block| { let child_info = match ChildType::from_prefixed_key(&storage_key) { - Some((ChildType::ParentKeyId, storage_key)) => - ChildInfo::new_default(storage_key), + Some((ChildType::ParentKeyId, storage_key)) => { + ChildInfo::new_default(storage_key) + } None => return Err(sp_blockchain::Error::InvalidChildStorageKey), }; self.client.child_storage_keys(&BlockId::Hash(block), &child_info, &prefix) @@ -659,8 +662,9 @@ where self.block_or_best(block) .and_then(|block| { let child_info = match ChildType::from_prefixed_key(&storage_key) { - Some((ChildType::ParentKeyId, storage_key)) => - ChildInfo::new_default(storage_key), + Some((ChildType::ParentKeyId, storage_key)) => { + ChildInfo::new_default(storage_key) + } None => return Err(sp_blockchain::Error::InvalidChildStorageKey), }; self.client.child_storage_keys_iter( @@ -683,8 +687,9 @@ where self.block_or_best(block) .and_then(|block| { let child_info = match ChildType::from_prefixed_key(&storage_key) { - Some((ChildType::ParentKeyId, storage_key)) => - ChildInfo::new_default(storage_key), + Some((ChildType::ParentKeyId, storage_key)) => { + ChildInfo::new_default(storage_key) + } None => return Err(sp_blockchain::Error::InvalidChildStorageKey), }; self.client.child_storage(&BlockId::Hash(block), &child_info, &key) @@ -699,8 +704,9 @@ where keys: Vec, ) -> std::result::Result>, Error> { let child_info = match ChildType::from_prefixed_key(&storage_key) { - Some((ChildType::ParentKeyId, storage_key)) => - Arc::new(ChildInfo::new_default(storage_key)), + Some((ChildType::ParentKeyId, storage_key)) => { + Arc::new(ChildInfo::new_default(storage_key)) + } None => return Err(client_err(sp_blockchain::Error::InvalidChildStorageKey)), }; let block = self.block_or_best(block).map_err(client_err)?; @@ -725,8 +731,9 @@ where self.block_or_best(block) .and_then(|block| { let child_info = match ChildType::from_prefixed_key(&storage_key) { - Some((ChildType::ParentKeyId, storage_key)) => - ChildInfo::new_default(storage_key), + Some((ChildType::ParentKeyId, storage_key)) => { + ChildInfo::new_default(storage_key) + } None => return Err(sp_blockchain::Error::InvalidChildStorageKey), }; self.client.child_storage_hash(&BlockId::Hash(block), &child_info, &key) diff --git a/client/rpc/src/state/state_light.rs b/client/rpc/src/state/state_light.rs index 7f534eb4f82d8..f0fe404aca62d 100644 --- a/client/rpc/src/state/state_light.rs +++ b/client/rpc/src/state/state_light.rs @@ -62,7 +62,7 @@ type StorageMap = HashMap>; #[derive(Clone)] pub struct LightState, Client> { client: Arc, - executor: Arc, + executor: SubscriptionTaskExecutor, version_subscriptions: SimpleSubscriptions, storage_subscriptions: Arc>>, remote_blockchain: Arc>, @@ -133,7 +133,7 @@ where /// Create new state API backend for light nodes. pub fn new( client: Arc, - executor: Arc, + executor: SubscriptionTaskExecutor, remote_blockchain: Arc>, fetcher: Arc, ) -> Self { @@ -430,10 +430,10 @@ where }); old_storage = Ok(new_value); res - }, + } false => None, } - }, + } _ => None, }; ready(res) @@ -465,7 +465,7 @@ where if entry.get().is_empty() { entry.remove(); } - }, + } } } } @@ -708,7 +708,7 @@ where // if that isn't the first request - just listen for existing request' response if !need_issue_request { - return Either::Right(receiver.then(|r| ready(r.unwrap_or(Err(()))))) + return Either::Right(receiver.then(|r| ready(r.unwrap_or(Err(()))))); } // that is the first request - issue remote request + notify all listeners on diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 2a5f59b1e3590..109a0eff13ff3 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -150,8 +150,9 @@ impl KeystoreContainer { /// Construct KeystoreContainer pub fn new(config: &KeystoreConfig) -> Result { let keystore = Arc::new(match config { - KeystoreConfig::Path { path, password } => - LocalKeystore::open(path.clone(), password.clone())?, + KeystoreConfig::Path { path, password } => { + LocalKeystore::open(path.clone(), password.clone())? + } KeystoreConfig::InMemory => LocalKeystore::in_memory(), }); @@ -424,7 +425,8 @@ pub struct SpawnTasksParams<'a, TBl: BlockT, TCl, TExPool, Backend> { /// A shared transaction pool. pub transaction_pool: Arc, /// Builds additional [`RpcModule`]s that should be added to the server - pub rpc_builder: Box) -> RpcModule<()>>, + pub rpc_builder: + Box Result, Error>>, /// An optional, shared remote blockchain instance. Used for light clients. pub remote_blockchain: Option>>, /// A shared network instance. @@ -651,8 +653,6 @@ fn init_telemetry>( Ok(telemetry.handle()) } -// Maciej: This is very WIP, mocking the original `gen_handler`. All of the `jsonrpsee` -// specific logic should be merged back to `gen_handler` down the road. fn gen_rpc_module( deny_unsafe: DenyUnsafe, spawn_handle: SpawnTaskHandle, @@ -664,8 +664,10 @@ fn gen_rpc_module( system_rpc_tx: TracingUnboundedSender>, config: &Configuration, offchain_storage: Option<>::OffchainStorage>, - rpc_builder: Box) -> RpcModule<()>>, -) -> RpcModule<()> + rpc_builder: Box< + dyn FnOnce(DenyUnsafe, SubscriptionTaskExecutor) -> Result, Error>, + >, +) -> Result, Error> where TBl: BlockT, TCl: ProvideRuntimeApi @@ -686,8 +688,6 @@ where TBl::Hash: Unpin, TBl::Header: Unpin, { - const UNIQUE_METHOD_NAMES_PROOF: &str = "Method names are unique; qed"; - let system_info = sc_rpc::system::SystemInfo { chain_name: config.chain_spec.name().into(), impl_name: config.impl_name.clone(), @@ -695,7 +695,7 @@ where properties: config.chain_spec.properties(), chain_type: config.chain_spec.chain_type(), }; - let task_executor = Arc::new(SubscriptionTaskExecutor::new(spawn_handle)); + let task_executor = SubscriptionTaskExecutor::new(spawn_handle); let mut rpc_api = RpcModule::new(()); @@ -747,19 +747,19 @@ where if let Some(storage) = offchain_storage { let offchain = sc_rpc::offchain::Offchain::new(storage, deny_unsafe).into_rpc(); - rpc_api.merge(offchain).expect(UNIQUE_METHOD_NAMES_PROOF); + rpc_api.merge(offchain).map_err(|e| Error::Application(e.into()))?; } - rpc_api.merge(chain).expect(UNIQUE_METHOD_NAMES_PROOF); - rpc_api.merge(author).expect(UNIQUE_METHOD_NAMES_PROOF); - rpc_api.merge(system).expect(UNIQUE_METHOD_NAMES_PROOF); - rpc_api.merge(state).expect(UNIQUE_METHOD_NAMES_PROOF); - rpc_api.merge(child_state).expect(UNIQUE_METHOD_NAMES_PROOF); + rpc_api.merge(chain).map_err(|e| Error::Application(e.into()))?; + rpc_api.merge(author).map_err(|e| Error::Application(e.into()))?; + rpc_api.merge(system).map_err(|e| Error::Application(e.into()))?; + rpc_api.merge(state).map_err(|e| Error::Application(e.into()))?; + rpc_api.merge(child_state).map_err(|e| Error::Application(e.into()))?; // Additional [`RpcModule`]s defined in the node to fit the specific blockchain - let extra_rpcs = rpc_builder(deny_unsafe, task_executor.clone()); - rpc_api.merge(extra_rpcs).expect(UNIQUE_METHOD_NAMES_PROOF); + let extra_rpcs = rpc_builder(deny_unsafe, task_executor.clone())?; + rpc_api.merge(extra_rpcs).map_err(|e| Error::Application(e.into()))?; - rpc_api + Ok(rpc_api) } /// Parameters to pass into `build_network`. @@ -842,8 +842,8 @@ where let (handler, protocol_config) = BlockRequestHandler::new( &protocol_id, client.clone(), - config.network.default_peers_set.in_peers as usize + - config.network.default_peers_set.out_peers as usize, + config.network.default_peers_set.in_peers as usize + + config.network.default_peers_set.out_peers as usize, ); spawn_handle.spawn("block_request_handler", handler.run()); protocol_config @@ -859,8 +859,8 @@ where let (handler, protocol_config) = StateRequestHandler::new( &protocol_id, client.clone(), - config.network.default_peers_set.in_peers as usize + - config.network.default_peers_set.out_peers as usize, + config.network.default_peers_set.in_peers as usize + + config.network.default_peers_set.out_peers as usize, ); spawn_handle.spawn("state_request_handler", handler.run()); protocol_config @@ -974,7 +974,7 @@ where ); // This `return` might seem unnecessary, but we don't want to make it look like // everything is working as normal even though the user is clearly misusing the API. - return + return; } future.await diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index 8535abb16bc2e..89c62fb2dda61 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -43,7 +43,6 @@ use log::{debug, error, warn}; use parity_util_mem::MallocSizeOf; use sc_client_api::{blockchain::HeaderBackend, BlockchainEvents}; use sc_network::PeerId; -use sc_rpc::{DenyUnsafe, SubscriptionTaskExecutor}; use sc_utils::mpsc::TracingUnboundedReceiver; use sp_runtime::{ generic::BlockId, @@ -109,8 +108,6 @@ pub struct PartialComponents, - /// RPC module builder. - pub rpc_builder: Box) -> RpcModule<()>>, /// Everything else that needs to be passed into the main build function. pub other: Other, } @@ -306,9 +303,9 @@ fn start_rpc_servers( gen_rpc_module: R, ) -> Result, error::Error> where - R: FnOnce(sc_rpc::DenyUnsafe) -> RpcModule<()>, + R: FnOnce(sc_rpc::DenyUnsafe) -> Result, Error>, { - let module = gen_rpc_module(sc_rpc::DenyUnsafe::Yes); + let module = gen_rpc_module(sc_rpc::DenyUnsafe::Yes)?; let ws_addr = config.rpc_ws.unwrap_or_else(|| "127.0.0.1:9944".parse().unwrap()); let http_addr = config.rpc_http.unwrap_or_else(|| "127.0.0.1:9933".parse().unwrap()); @@ -388,8 +385,8 @@ where Ok(uxt) => uxt, Err(e) => { debug!("Transaction invalid: {:?}", e); - return Box::pin(futures::future::ready(TransactionImport::Bad)) - }, + return Box::pin(futures::future::ready(TransactionImport::Bad)); + } }; let best_block_id = BlockId::hash(self.client.info().best_hash); @@ -403,18 +400,19 @@ where match import_future.await { Ok(_) => TransactionImport::NewGood, Err(e) => match e.into_pool_error() { - Ok(sc_transaction_pool_api::error::Error::AlreadyImported(_)) => - TransactionImport::KnownGood, + Ok(sc_transaction_pool_api::error::Error::AlreadyImported(_)) => { + TransactionImport::KnownGood + } Ok(e) => { debug!("Error adding transaction to the pool: {:?}", e); TransactionImport::Bad - }, + } Err(e) => { debug!("Error converting pool error: {:?}", e); // it is not bad at least, just some internal node logic error, so peer is // innocent. TransactionImport::KnownGood - }, + } }, } }) diff --git a/frame/contracts/rpc/Cargo.toml b/frame/contracts/rpc/Cargo.toml index 477c5ad55ebb1..0e403820ae0b3 100644 --- a/frame/contracts/rpc/Cargo.toml +++ b/frame/contracts/rpc/Cargo.toml @@ -14,6 +14,7 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] codec = { package = "parity-scale-codec", version = "2" } +anyhow = "1" jsonrpsee = { git = "https://github.com/paritytech/jsonrpsee", branch = "master", features = ["server"] } log = "0.4" serde = { version = "1", features = ["derive"] } diff --git a/frame/contracts/rpc/src/lib.rs b/frame/contracts/rpc/src/lib.rs index 275e499fed091..403fc6bf7c3b8 100644 --- a/frame/contracts/rpc/src/lib.rs +++ b/frame/contracts/rpc/src/lib.rs @@ -21,6 +21,7 @@ use std::{marker::PhantomData, sync::Arc}; +use anyhow::anyhow; use codec::Codec; use jsonrpsee::{ proc_macros::rpc, @@ -256,26 +257,17 @@ fn decode_hex>( from: H, name: &str, ) -> Result { - from.try_into().map_err(|_| { - CallError::Custom { - code: -32602, // TODO: was `ErrorCode::InvalidParams` - message: format!("{:?} does not fit into the {} type", from, name), - data: None, - } - .into() - }) + from.try_into() + .map_err(|_| anyhow!("{:?} does not fit into the {} type", from, name).into()) } fn limit_gas(gas_limit: Weight) -> Result<(), JsonRpseeError> { if gas_limit > GAS_LIMIT { - Err(CallError::Custom { - code: -32602, // TODO: was `ErrorCode::InvalidParams,` - message: format!( - "Requested gas limit is greater than maximum allowed: {} > {}", - gas_limit, GAS_LIMIT - ), - data: None, - } + Err(anyhow!( + "Requested gas limit is greater than maximum allowed: {} > {}", + gas_limit, + GAS_LIMIT + ) .into()) } else { Ok(()) diff --git a/test-utils/test-runner/src/client.rs b/test-utils/test-runner/src/client.rs index 4b70fbea03124..71069d589ff84 100644 --- a/test-utils/test-runner/src/client.rs +++ b/test-utils/test-runner/src/client.rs @@ -102,8 +102,9 @@ where use sp_consensus_babe::AuthorityId; let config = match config_or_chain_spec { ConfigOrChainSpec::Config(config) => config, - ConfigOrChainSpec::ChainSpec(chain_spec, tokio_handle) => - default_config(tokio_handle, chain_spec), + ConfigOrChainSpec::ChainSpec(chain_spec, tokio_handle) => { + default_config(tokio_handle, chain_spec) + } }; let executor = NativeElseWasmExecutor::::new( @@ -186,11 +187,11 @@ where let (command_sink, commands_stream) = mpsc::channel(10); let rpc_sink = command_sink.clone(); - let rpc_builder = Box::new(move |_, _| -> RpcModule<()> { + let rpc_builder = Box::new(move |_, _| { let seal = ManualSeal::new(rpc_sink).into_rpc(); let mut module = RpcModule::new(()); module.merge(seal).expect("only one module; qed"); - module + Ok(module) }); let _rpc_handlers = {