Refactor sr-api to not depend on client anymore (#4086)

* Refactor sr-api to not depend on client anymore

* Fix benches

* Apply suggestions from code review

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* Apply suggestions from code review

substrate/core/consensus/aura/Cargo.toml

@@ -20,13 +20,15 @@ client = { package = "substrate-client", path = "../../client" }
 substrate-telemetry = { path = "../../telemetry" }
 keystore = { package = "substrate-keystore", path = "../../keystore" }
 consensus_common = { package = "substrate-consensus-common", path = "../common" }
-sr-primitives = {  path = "../../sr-primitives" }
+sr-primitives = { path = "../../sr-primitives" }
+sr-api = { path = "../../sr-api" }
 futures-preview = { version = "0.3.0-alpha.19", features = ["compat"] }
 futures01 = { package = "futures", version = "0.1" }
 futures-timer = "0.4.0"
 parking_lot = "0.9.0"
 log = "0.4.8"
 derive_more = "0.15.0"
+block-builder-api = { package = "substrate-block-builder-runtime-api", path = "../../block-builder/runtime-api" }
 
 [dev-dependencies]
 keyring = { package = "substrate-keyring", path = "../../keyring" }

substrate/core/consensus/aura/primitives/Cargo.toml

@@ -7,7 +7,7 @@ edition = "2018"
 
 [dependencies]
 codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false }
-substrate-client = { path = "../../../client", default-features = false }
+sr-api = { path = "../../../sr-api", default-features = false }
 app-crypto = { package = "substrate-application-crypto", path = "../../../application-crypto", default-features = false }
 rstd = { package = "sr-std", path = "../../../sr-std", default-features = false }
 sr-primitives = { path = "../../../sr-primitives", default-features = false }
@@ -18,6 +18,6 @@ std = [
 	"rstd/std",
 	"codec/std",
 	"sr-primitives/std",
-	"substrate-client/std",
+	"sr-api/std",
 	"app-crypto/std",
 ]

substrate/core/consensus/aura/primitives/src/lib.rs

@@ -19,7 +19,6 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 
 use codec::{Encode, Decode, Codec};
-use substrate_client::decl_runtime_apis;
 use rstd::vec::Vec;
 use sr_primitives::ConsensusEngineId;
 
@@ -74,7 +73,7 @@ pub enum ConsensusLog<AuthorityId: Codec> {
 	OnDisabled(AuthorityIndex),
 }
 
-decl_runtime_apis! {
+sr_api::decl_runtime_apis! {
 	/// API necessary for block authorship with aura.
 	pub trait AuraApi<AuthorityId: Codec> {
 		/// Return the slot duration in seconds for Aura.

substrate/core/consensus/aura/src/lib.rs

@@ -39,16 +39,17 @@ use consensus_common::import_queue::{
 	Verifier, BasicQueue, BoxBlockImport, BoxJustificationImport, BoxFinalityProofImport,
 };
 use client::{
-	block_builder::api::BlockBuilder as BlockBuilderApi, blockchain::ProvideCache,
-	runtime_api::ApiExt, error::Result as CResult, backend::AuxStore, BlockOf,
+	blockchain::ProvideCache, error::Result as CResult, backend::AuxStore, BlockOf,
 	well_known_cache_keys::{self, Id as CacheKeyId},
 };
 
+use block_builder_api::BlockBuilder as BlockBuilderApi;
+
 use sr_primitives::{generic::{BlockId, OpaqueDigestItemId}, Justification};
 use sr_primitives::traits::{Block as BlockT, Header, DigestItemFor, ProvideRuntimeApi, Zero, Member};
 
 use primitives::crypto::Pair;
-use inherents::{InherentDataProviders, InherentData, RuntimeString};
+use inherents::{InherentDataProviders, InherentData};
 
 use futures::prelude::*;
 use parking_lot::Mutex;
@@ -65,6 +66,8 @@ use slots::check_equivocation;
 
 use keystore::KeyStorePtr;
 
+use sr_api::ApiExt;
+
 pub use aura_primitives::*;
 pub use consensus_common::SyncOracle;
 pub use digest::CompatibleDigestItem;
@@ -85,7 +88,7 @@ impl SlotDuration {
 		A: Codec,
 		B: BlockT,
 		C: AuxStore + ProvideRuntimeApi,
-		C::Api: AuraApi<B, A>,
+		C::Api: AuraApi<B, A, Error = client::error::Error>,
 	{
 		slots::SlotDuration::get_or_compute(client, |a, b| a.slot_duration(b)).map(Self)
 	}
@@ -332,7 +335,7 @@ enum Error<B: BlockT> {
 	TooFarInFuture,
 	Client(client::error::Error),
 	DataProvider(String),
-	Runtime(RuntimeString)
+	Runtime(String),
 }
 
 fn find_pre_digest<B: BlockT, P: Pair>(header: &B::Header) -> Result<u64, Error<B>>
@@ -438,7 +441,7 @@ impl<C, P, T> AuraVerifier<C, P, T>
 		inherent_data: InherentData,
 		timestamp_now: u64,
 	) -> Result<(), Error<B>>
-		where C: ProvideRuntimeApi, C::Api: BlockBuilderApi<B>
+		where C: ProvideRuntimeApi, C::Api: BlockBuilderApi<B, Error = client::error::Error>
 	{
 		const MAX_TIMESTAMP_DRIFT_SECS: u64 = 60;
 
@@ -471,7 +474,7 @@ impl<C, P, T> AuraVerifier<C, P, T>
 						thread::sleep(Duration::from_secs(diff));
 						Ok(())
 					},
-					Some(TIError::Other(e)) => Err(Error::Runtime(e)),
+					Some(TIError::Other(e)) => Err(Error::Runtime(e.into())),
 					None => Err(Error::DataProvider(
 						self.inherent_data_providers.error_to_string(&i, &e)
 					)),
@@ -485,7 +488,7 @@ impl<C, P, T> AuraVerifier<C, P, T>
 #[forbid(deprecated)]
 impl<B: BlockT, C, P, T> Verifier<B> for AuraVerifier<C, P, T> where
 	C: ProvideRuntimeApi + Send + Sync + client::backend::AuxStore + ProvideCache<B> + BlockOf,
-	C::Api: BlockBuilderApi<B> + AuraApi<B, AuthorityId<P>>,
+	C::Api: BlockBuilderApi<B> + AuraApi<B, AuthorityId<P>> + ApiExt<B, Error = client::error::Error>,
 	DigestItemFor<B>: CompatibleDigestItem<P>,
 	P: Pair + Send + Sync + 'static,
 	P::Public: Send + Sync + Hash + Eq + Clone + Decode + Encode + Debug + 'static,
@@ -499,7 +502,9 @@ impl<B: BlockT, C, P, T> Verifier<B> for AuraVerifier<C, P, T> where
 		justification: Option<Justification>,
 		mut body: Option<Vec<B::Extrinsic>>,
 	) -> Result<(BlockImportParams<B>, Option<Vec<(CacheKeyId, Vec<u8>)>>), String> {
-		let mut inherent_data = self.inherent_data_providers.create_inherent_data().map_err(String::from)?;
+		let mut inherent_data = self.inherent_data_providers
+			.create_inherent_data()
+			.map_err(|e| e.into_string())?;
 		let (timestamp_now, slot_now, _) = AuraSlotCompatible.extract_timestamp_and_slot(&inherent_data)
 			.map_err(|e| format!("Could not extract timestamp and slot: {:?}", e))?;
 		let hash = header.hash();
@@ -530,7 +535,10 @@ impl<B: BlockT, C, P, T> Verifier<B> for AuraVerifier<C, P, T> where
 					// skip the inherents verification if the runtime API is old.
 					if self.client
 						.runtime_api()
-						.has_api_with::<dyn BlockBuilderApi<B>, _>(&BlockId::Hash(parent_hash), |v| v >= 2)
+						.has_api_with::<dyn BlockBuilderApi<B, Error = ()>, _>(
+							&BlockId::Hash(parent_hash),
+							|v| v >= 2,
+						)
 						.map_err(|e| format!("{:?}", e))?
 					{
 						self.check_inherents(
@@ -667,7 +675,7 @@ pub fn import_queue<B, C, P, T>(
 ) -> Result<AuraImportQueue<B>, consensus_common::Error> where
 	B: BlockT,
 	C: 'static + ProvideRuntimeApi + BlockOf + ProvideCache<B> + Send + Sync + AuxStore,
-	C::Api: BlockBuilderApi<B> + AuraApi<B, AuthorityId<P>>,
+	C::Api: BlockBuilderApi<B> + AuraApi<B, AuthorityId<P>> + ApiExt<B, Error = client::error::Error>,
 	DigestItemFor<B>: CompatibleDigestItem<P>,
 	P: Pair + Send + Sync + 'static,
 	P::Public: Clone + Eq + Send + Sync + Hash + Debug + Encode + Decode,

substrate/core/consensus/babe/Cargo.toml

@@ -21,6 +21,8 @@ substrate-telemetry = { path = "../../telemetry" }
 keystore = { package = "substrate-keystore", path = "../../keystore" }
 srml-babe = { path = "../../../srml/babe" }
 client = { package = "substrate-client", path = "../../client" }
+sr-api = { path = "../../sr-api" }
+block-builder-api = { package = "substrate-block-builder-runtime-api", path = "../../block-builder/runtime-api" }
 header-metadata = { package = "substrate-header-metadata", path = "../../client/header-metadata" }
 consensus-common = { package = "substrate-consensus-common", path = "../common" }
 uncles = { package = "substrate-consensus-uncles", path = "../uncles" }
@@ -44,6 +46,7 @@ substrate-executor = { path = "../../executor" }
 network = { package = "substrate-network", path = "../../network", features = ["test-helpers"]}
 service = { package = "substrate-service", path = "../../service" }
 test-client = { package = "substrate-test-runtime-client", path = "../../test-runtime/client" }
+block-builder = { package = "substrate-block-builder", path = "../../block-builder" }
 tokio = "0.1.22"
 env_logger = "0.7.0"
 tempfile = "3.1.0"

substrate/core/consensus/babe/primitives/Cargo.toml

@@ -6,7 +6,7 @@ description = "Primitives for BABE consensus"
 edition = "2018"
 
 [dependencies]
-substrate-client = { path = "../../../client", default-features = false }
+sr-api = { path = "../../../sr-api", default-features = false }
 rstd = { package = "sr-std", path = "../../../sr-std", default-features = false }
 sr-primitives = {  path = "../../../sr-primitives", default-features = false }
 app-crypto = { package = "substrate-application-crypto", path = "../../../application-crypto", default-features = false }
@@ -19,7 +19,7 @@ default = ["std"]
 std = [
 	"rstd/std",
 	"sr-primitives/std",
-	"substrate-client/std",
+	"sr-api/std",
 	"codec/std",
 	"schnorrkel",
 	"slots",

substrate/core/consensus/babe/primitives/src/lib.rs

@@ -24,7 +24,6 @@ mod digest;
 use codec::{Encode, Decode};
 use rstd::vec::Vec;
 use sr_primitives::{ConsensusEngineId, RuntimeDebug};
-use substrate_client::decl_runtime_apis;
 
 #[cfg(feature = "std")]
 pub use digest::{BabePreDigest, CompatibleDigestItem};
@@ -165,7 +164,7 @@ impl slots::SlotData for BabeConfiguration {
 	const SLOT_KEY: &'static [u8] = b"babe_configuration";
 }
 
-decl_runtime_apis! {
+sr_api::decl_runtime_apis! {
 	/// API necessary for block authorship with BABE.
 	pub trait BabeApi {
 		/// Return the configuration for BABE. Currently,

substrate/core/consensus/babe/src/lib.rs

@@ -66,7 +66,7 @@ use consensus_common::ImportResult;
 use consensus_common::import_queue::{
 	BoxJustificationImport, BoxFinalityProofImport,
 };
-use sr_primitives::{generic::{BlockId, OpaqueDigestItemId}, Justification, RuntimeString};
+use sr_primitives::{generic::{BlockId, OpaqueDigestItemId}, Justification};
 use sr_primitives::traits::{
 	Block as BlockT, Header, DigestItemFor, ProvideRuntimeApi,
 	Zero,
@@ -75,11 +75,7 @@ use keystore::KeyStorePtr;
 use parking_lot::Mutex;
 use primitives::{Blake2Hasher, H256, Pair};
 use inherents::{InherentDataProviders, InherentData};
-use substrate_telemetry::{
-	telemetry,
-	CONSENSUS_TRACE,
-	CONSENSUS_DEBUG,
-};
+use substrate_telemetry::{telemetry, CONSENSUS_TRACE, CONSENSUS_DEBUG};
 use consensus_common::{
 	self, BlockImport, Environment, Proposer, BlockCheckParams,
 	ForkChoiceStrategy, BlockImportParams, BlockOrigin, Error as ConsensusError,
@@ -91,11 +87,13 @@ use srml_babe::{
 use consensus_common::SelectChain;
 use consensus_common::import_queue::{Verifier, BasicQueue, CacheKeyId};
 use client::{
-	block_builder::api::BlockBuilder as BlockBuilderApi,
 	blockchain::{self, HeaderBackend, ProvideCache}, BlockchainEvents, CallExecutor, Client,
 	error::Result as ClientResult, error::Error as ClientError, backend::{AuxStore, Backend},
 	ProvideUncles,
 };
+
+use block_builder_api::BlockBuilder as BlockBuilderApi;
+
 use slots::{CheckedHeader, check_equivocation};
 use futures::prelude::*;
 use log::{warn, debug, info, trace};
@@ -104,6 +102,8 @@ use epoch_changes::descendent_query;
 use header_metadata::HeaderMetadata;
 use schnorrkel::SignatureError;
 
+use sr_api::ApiExt;
+
 mod aux_schema;
 mod verification;
 mod epoch_changes;
@@ -167,7 +167,7 @@ enum Error<B: BlockT> {
 	#[display(fmt = "Checking inherents failed: {}", _0)]
 	CheckInherents(String),
 	Client(client::error::Error),
-	Runtime(RuntimeString),
+	Runtime(inherents::Error),
 	ForkTree(Box<fork_tree::Error<client::error::Error>>),
 }
 
@@ -202,7 +202,7 @@ impl Config {
 	/// Either fetch the slot duration from disk or compute it from the genesis
 	/// state.
 	pub fn get_or_compute<B: BlockT, C>(client: &C) -> ClientResult<Self> where
-		C: AuxStore + ProvideRuntimeApi, C::Api: BabeApi<B>,
+		C: AuxStore + ProvideRuntimeApi, C::Api: BabeApi<B, Error = client::error::Error>,
 	{
 		trace!(target: "babe", "Getting slot duration");
 		match slots::SlotDuration::get_or_compute(client, |a, b| a.configuration(b)).map(Self) {
@@ -554,7 +554,7 @@ impl<B, E, Block: BlockT, RA, PRA> BabeVerifier<B, E, Block, RA, PRA> {
 		block_id: BlockId<Block>,
 		inherent_data: InherentData,
 	) -> Result<(), Error<Block>>
-		where PRA: ProvideRuntimeApi, PRA::Api: BlockBuilderApi<Block>
+		where PRA: ProvideRuntimeApi, PRA::Api: BlockBuilderApi<Block, Error = client::error::Error>
 	{
 		let inherent_res = self.api.runtime_api().check_inherents(
 			&block_id,
@@ -623,7 +623,8 @@ impl<B, E, Block, RA, PRA> Verifier<Block> for BabeVerifier<B, E, Block, RA, PRA
 	E: CallExecutor<Block, Blake2Hasher> + 'static + Clone + Send + Sync,
 	RA: Send + Sync,
 	PRA: ProvideRuntimeApi + Send + Sync + AuxStore + ProvideCache<Block>,
-	PRA::Api: BlockBuilderApi<Block> + BabeApi<Block>,
+	PRA::Api: BlockBuilderApi<Block, Error = client::error::Error>
+		+ BabeApi<Block, Error = client::error::Error>,
 {
 	fn verify(
 		&mut self,
@@ -1141,7 +1142,7 @@ pub fn import_queue<B, E, Block: BlockT<Hash=H256>, I, RA, PRA>(
 	E: CallExecutor<Block, Blake2Hasher> + Clone + Send + Sync + 'static,
 	RA: Send + Sync + 'static,
 	PRA: ProvideRuntimeApi + ProvideCache<Block> + Send + Sync + AuxStore + 'static,
-	PRA::Api: BlockBuilderApi<Block> + BabeApi<Block>,
+	PRA::Api: BlockBuilderApi<Block> + BabeApi<Block> + ApiExt<Block, Error = client::error::Error>,
 {
 	register_babe_inherent_data_provider(&inherent_data_providers, babe_link.config.slot_duration)?;
 

substrate/core/consensus/babe/src/tests.rs

@@ -23,7 +23,7 @@ use super::*;
 use authorship::claim_slot;
 
 use babe_primitives::{AuthorityPair, SlotNumber};
-use client::block_builder::BlockBuilder;
+use block_builder::BlockBuilder;
 use consensus_common::NoNetwork as DummyOracle;
 use consensus_common::import_queue::{
 	BoxBlockImport, BoxJustificationImport, BoxFinalityProofImport,

substrate/core/consensus/common/src/error.rs

@@ -36,7 +36,7 @@ pub enum Error {
 	FaultyTimer(std::io::Error),
 	/// Error while working with inherent data.
 	#[display(fmt="InherentData error: {}", _0)]
-	InherentData(String),
+	InherentData(inherents::Error),
 	/// Unable to propose a block.
 	#[display(fmt="Unable to create block proposal.")]
 	CannotPropose,

substrate/core/consensus/pow/Cargo.toml

@@ -10,6 +10,7 @@ codec = { package = "parity-scale-codec", version = "1.0.0", features = ["derive
 primitives = { package = "substrate-primitives", path = "../../primitives" }
 sr-primitives = { path = "../../sr-primitives" }
 client = { package = "substrate-client", path = "../../client" }
+block-builder-api = { package = "substrate-block-builder-runtime-api", path = "../../block-builder/runtime-api" }
 srml-timestamp = { path = "../../../srml/timestamp" }
 inherents = { package = "substrate-inherents", path = "../../inherents" }
 pow-primitives = { package = "substrate-consensus-pow-primitives", path = "primitives" }

substrate/core/consensus/pow/primitives/Cargo.toml

@@ -6,7 +6,7 @@ description = "Primitives for Aura consensus"
 edition = "2018"
 
 [dependencies]
-substrate-client = { path = "../../../client", default-features = false }
+sr-api = { path = "../../../sr-api", default-features = false }
 rstd = { package = "sr-std", path = "../../../sr-std", default-features = false }
 sr-primitives = { path = "../../../sr-primitives", default-features = false }
 primitives = { package = "substrate-primitives", path = "../../../primitives", default-features = false }
@@ -16,7 +16,7 @@ codec = { package = "parity-scale-codec", version = "1.0.0", default-features =
 default = ["std"]
 std = [
 	"rstd/std",
-	"substrate-client/std",
+	"sr-api/std",
 	"sr-primitives/std",
 	"primitives/std",
 	"codec/std",

substrate/core/consensus/pow/primitives/src/lib.rs

@@ -21,7 +21,6 @@
 use rstd::vec::Vec;
 use sr_primitives::ConsensusEngineId;
 use codec::Decode;
-use substrate_client::decl_runtime_apis;
 
 /// The `ConsensusEngineId` of PoW.
 pub const POW_ENGINE_ID: ConsensusEngineId = [b'p', b'o', b'w', b'_'];
@@ -48,7 +47,7 @@ impl TotalDifficulty for u128 {
 	}
 }
 
-decl_runtime_apis! {
+sr_api::decl_runtime_apis! {
 	/// API necessary for timestamp-based difficulty adjustment algorithms.
 	pub trait TimestampApi<Moment: Decode> {
 		/// Return the timestamp in the current block.

substrate/core/consensus/pow/src/lib.rs

@@ -33,10 +33,10 @@ use std::sync::Arc;
 use std::thread;
 use std::collections::HashMap;
 use client::{
-	BlockOf, blockchain::{HeaderBackend, ProvideCache},
-	block_builder::api::BlockBuilder as BlockBuilderApi, backend::AuxStore,
+	BlockOf, blockchain::{HeaderBackend, ProvideCache}, backend::AuxStore,
 	well_known_cache_keys::Id as CacheKeyId,
 };
+use block_builder_api::BlockBuilder as BlockBuilderApi;
 use sr_primitives::{Justification, RuntimeString};
 use sr_primitives::generic::{BlockId, Digest, DigestItem};
 use sr_primitives::traits::{Block as BlockT, Header as HeaderT, ProvideRuntimeApi};
@@ -75,7 +75,7 @@ pub enum Error<B: BlockT> {
 	#[display(fmt = "Error with block built on {:?}: {:?}", _0, _1)]
 	BlockBuiltError(B::Hash, ConsensusError),
 	#[display(fmt = "Creating inherents failed: {}", _0)]
-	CreateInherents(RuntimeString),
+	CreateInherents(inherents::Error),
 	#[display(fmt = "Checking inherents failed: {}", _0)]
 	CheckInherents(String),
 	Client(client::error::Error),
@@ -210,7 +210,7 @@ impl<B: BlockT<Hash=H256>, C, S, Algorithm> PowVerifier<B, C, S, Algorithm> {
 		inherent_data: InherentData,
 		timestamp_now: u64,
 	) -> Result<(), Error<B>> where
-		C: ProvideRuntimeApi, C::Api: BlockBuilderApi<B>
+		C: ProvideRuntimeApi, C::Api: BlockBuilderApi<B, Error = client::error::Error>
 	{
 		const MAX_TIMESTAMP_DRIFT_SECS: u64 = 60;
 
@@ -248,7 +248,7 @@ impl<B: BlockT<Hash=H256>, C, S, Algorithm> PowVerifier<B, C, S, Algorithm> {
 
 impl<B: BlockT<Hash=H256>, C, S, Algorithm> Verifier<B> for PowVerifier<B, C, S, Algorithm> where
 	C: ProvideRuntimeApi + Send + Sync + HeaderBackend<B> + AuxStore + ProvideCache<B> + BlockOf,
-	C::Api: BlockBuilderApi<B>,
+	C::Api: BlockBuilderApi<B, Error = client::error::Error>,
 	S: SelectChain<B>,
 	Algorithm: PowAlgorithm<B> + Send + Sync,
 {
@@ -260,8 +260,8 @@ impl<B: BlockT<Hash=H256>, C, S, Algorithm> Verifier<B> for PowVerifier<B, C, S,
 		mut body: Option<Vec<B::Extrinsic>>,
 	) -> Result<(BlockImportParams<B>, Option<Vec<(CacheKeyId, Vec<u8>)>>), String> {
 		let inherent_data = self.inherent_data_providers
-			.create_inherent_data().map_err(String::from)?;
-		let timestamp_now = inherent_data.timestamp_inherent_data().map_err(String::from)?;
+			.create_inherent_data().map_err(|e| e.into_string())?;
+		let timestamp_now = inherent_data.timestamp_inherent_data().map_err(|e| e.into_string())?;
 
 		let best_hash = match self.select_chain.as_ref() {
 			Some(select_chain) => select_chain.best_chain()
@@ -340,7 +340,7 @@ pub fn import_queue<B, C, S, Algorithm>(
 	B: BlockT<Hash=H256>,
 	C: ProvideRuntimeApi + HeaderBackend<B> + BlockOf + ProvideCache<B> + AuxStore,
 	C: Send + Sync + AuxStore + 'static,
-	C::Api: BlockBuilderApi<B>,
+	C::Api: BlockBuilderApi<B, Error = client::error::Error>,
 	Algorithm: PowAlgorithm<B> + Send + Sync + 'static,
 	S: SelectChain<B> + 'static,
 {

substrate/core/consensus/slots/src/slots.rs

@@ -146,7 +146,7 @@ impl<SC: SlotCompatible + Unpin> Stream for Slots<SC> {
 
 			let inherent_data = match self.inherent_data_providers.create_inherent_data() {
 				Ok(id) => id,
-				Err(err) => return Poll::Ready(Some(Err(consensus_common::Error::InherentData(err.into_owned())))),
+				Err(err) => return Poll::Ready(Some(Err(consensus_common::Error::InherentData(err)))),
 			};
 			let result = self.timestamp_extractor.extract_timestamp_and_slot(&inherent_data);
 			let (timestamp, slot_num, offset) = match result {