-
Notifications
You must be signed in to change notification settings - Fork 14
Trappist node refactor #177
Comments
I'm encountering an issue, which has led me to understand why there's a certain amount of duplication in the current code of the node. Let's delve into the composition of the service file: Essentially, there are three functions that come into play when the commands for starting the node are invoked (command.rs): new_partial<RuntimeApi, BIQ> The challenge I'm currently encountering is that Stout and Trappist have different RuntimeApi configurations. This difference is due to the runtime APIs for the DEX that exist on Trappist but not on Stout. If we examine how the RuntimeApi generics are set for the new_partial function, we can observe the following bounds: pub fn new_partial<RuntimeApi, BIQ>(
config: &Configuration,
build_import_queue: BIQ,
) -> Result<
PartialComponents<
ParachainClient<RuntimeApi>,
ParachainBackend,
(),
sc_consensus::DefaultImportQueue<Block, ParachainClient<RuntimeApi>>,
sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>,
(ParachainBlockImport<RuntimeApi>, Option<Telemetry>, Option<TelemetryWorkerHandle>),
>,
sc_service::Error,
>
where
RuntimeApi: ConstructRuntimeApi<Block, ParachainClient<RuntimeApi>> + Send + Sync + 'static,
RuntimeApi::RuntimeApi: sp_transaction_pool::runtime_api::TaggedTransactionQueue<Block>
+ sp_api::Metadata<Block>
+ sp_session::SessionKeys<Block>
+ sp_api::ApiExt<
Block,
StateBackend = sc_client_api::StateBackendFor<ParachainBackend, Block>,
> + sp_offchain::OffchainWorkerApi<Block>
+ sp_block_builder::BlockBuilder<Block>,
sc_client_api::StateBackendFor<ParachainBackend, Block>: sp_api::StateBackend<BlakeTwo256>,
BIQ: FnOnce(
Arc<ParachainClient<RuntimeApi>>,
ParachainBlockImport<RuntimeApi>,
&Configuration,
Option<TelemetryHandle>,
&TaskManager,
) -> Result<
sc_consensus::DefaultImportQueue<Block, ParachainClient<RuntimeApi>>,
sc_service::Error,
>, The specific part I want to highlight is RuntimeApi::RuntimeApi: sp_transaction_pool::runtime_api::TaggedTransactionQueue<Block>
+ sp_api::Metadata<Block>
+ sp_session::SessionKeys<Block>
+ sp_api::ApiExt<
Block,
StateBackend = sc_client_api::StateBackendFor<ParachainBackend, Block>,
> + sp_offchain::OffchainWorkerApi<Block>
+ sp_block_builder::BlockBuilder<Block>
+ cumulus_primitives_core::CollectCollationInfo<Block>
+ pallet_transaction_payment_rpc::TransactionPaymentRuntimeApi<Block, Balance>
+ frame_rpc_system::AccountNonceApi<Block, AccountId, Nonce>
+ pallet_dex_rpc::DexRuntimeApi<Block, AssetId, Balance, Balance>, It's important to note the sequence of functions where the RPC is built, specifically at this location (link): async fn start_node_impl<RuntimeApi, RB, BIQ, BIC>(
...
let rpc_builder = {
let client = client.clone();
let transaction_pool = transaction_pool.clone();
let backend_for_rpc = backend.clone();
Box::new(move |deny_unsafe, _| {
let deps = rpc::FullDeps {
client: client.clone(),
pool: transaction_pool.clone(),
deny_unsafe,
};
rpc::create_full(deps, backend_for_rpc.clone()).map_err(Into::into)
})
};
...
} Now that some background has been covered, the issue I'm facing right now is that if I add the trait bound There are a couple of options I'm considering to resolve this issue:
My preferred option is the option 1) but at least investing some time in thinking the best way to abstract behaviors and do not duplicate code. What do you think @stiiifff @kalaninja @valentinfernandez1 ? |
I forgot to mention that in case we go for the option 1) we could create one |
One potential approach is to rely on generics runtime api. Here is an example for the pub trait BaseRuntimeApi<RuntimeApi>:
ConstructRuntimeApi<Block, ParachainClient<RuntimeApi>> + Send + Sync + 'static
where
RuntimeApi: sp_transaction_pool::runtime_api::TaggedTransactionQueue<Block>
+ sp_api::Metadata<Block>
+ sp_session::SessionKeys<Block>
+ sp_api::ApiExt<
Block,
StateBackend = sc_client_api::StateBackendFor<ParachainBackend, Block>,
> + sp_offchain::OffchainWorkerApi<Block>
+ sp_block_builder::BlockBuilder<Block>,
sc_client_api::StateBackendFor<ParachainBackend, Block>: sp_api::StateBackend<BlakeTwo256>,
{
}
pub trait DexRuntimeApi<RuntimeApi>: BaseRuntimeApi<RuntimeApi>
where
RuntimeApi: pallet_dex_rpc::DexRuntimeApi<Block, AssetId, Balance, Balance>,
{
}
pub fn new_generic_partial<RuntimeApi: BaseRuntimeApi, BIQ>(
config: &Configuration,
build_import_queue: BIQ,
) -> Result<
PartialComponents<
ParachainClient<RuntimeApi>,
ParachainBackend,
(),
sc_consensus::DefaultImportQueue<Block, ParachainClient<RuntimeApi>>,
sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>,
(ParachainBlockImport<RuntimeApi>, Option<Telemetry>, Option<TelemetryWorkerHandle>),
>,
sc_service::Error,
> {
let telemetry = config
.telemetry_endpoints
.clone()
.filter(|x| !x.is_empty())
.map(|endpoints| -> Result<_, sc_telemetry::Error> {
let worker = TelemetryWorker::new(16)?;
let telemetry = worker.handle().new_telemetry(endpoints);
Ok((worker, telemetry))
})
.transpose()?;
let executor = sc_executor::WasmExecutor::<HostFunctions>::new(
config.wasm_method,
config.default_heap_pages,
config.max_runtime_instances,
None,
config.runtime_cache_size,
);
let (client, backend, keystore_container, task_manager) =
sc_service::new_full_parts::<Block, RuntimeApi, _>(
config,
telemetry.as_ref().map(|(_, telemetry)| telemetry.handle()),
executor,
)?;
let client = Arc::new(client);
let telemetry_worker_handle = telemetry.as_ref().map(|(worker, _)| worker.handle());
let telemetry = telemetry.map(|(worker, telemetry)| {
task_manager.spawn_handle().spawn("telemetry", None, worker.run());
telemetry
});
let transaction_pool = sc_transaction_pool::BasicPool::new_full(
config.transaction_pool.clone(),
config.role.is_authority().into(),
config.prometheus_registry(),
task_manager.spawn_essential_handle(),
client.clone(),
);
let block_import = ParachainBlockImport::new(client.clone(), backend.clone());
let import_queue = build_import_queue(
client.clone(),
block_import.clone(),
config,
telemetry.as_ref().map(|telemetry| telemetry.handle()),
&task_manager,
)?;
let params = PartialComponents {
backend,
client,
import_queue,
keystore_container,
task_manager,
transaction_pool,
select_chain: (),
other: (block_import, telemetry, telemetry_worker_handle),
};
Ok(params)
}
pub fn new_partial<RuntimeApi: DexRuntimeApi, BIQ>(
config: &Configuration,
build_import_queue: BIQ,
) -> Result<
PartialComponents<
ParachainClient<RuntimeApi>,
ParachainBackend,
(),
sc_consensus::DefaultImportQueue<Block, ParachainClient<RuntimeApi>>,
sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>,
(ParachainBlockImport<RuntimeApi>, Option<Telemetry>, Option<TelemetryWorkerHandle>),
>,
sc_service::Error,
> {
new_generic_partial::<RuntimeApi, BIQ>(config, build_import_queue)
}
pub fn new_stout_partial<RuntimeApi: BaseRuntimeApi, BIQ>(
config: &Configuration,
build_import_queue: BIQ,
) -> Result<
PartialComponents<
ParachainClient<RuntimeApi>,
ParachainBackend,
(),
sc_consensus::DefaultImportQueue<Block, ParachainClient<RuntimeApi>>,
sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>,
(ParachainBlockImport<RuntimeApi>, Option<Telemetry>, Option<TelemetryWorkerHandle>),
>,
sc_service::Error,
> {
new_generic_partial::<RuntimeApi, BIQ>(config, build_import_queue)
} |
So maybe this is a dumb question but why do we need the trait bound to |
Because it's needed to make to work the following line in the rpc create_full function: pub fn create_full<C, P, B>(
deps: FullDeps<C, P>,
backend: Arc<B>,
) -> Result<RpcExtension, Box<dyn std::error::Error + Send + Sync>>
where
....
C::Api: pallet_dex_rpc::DexRuntimeApi<
trappist_runtime::opaque::Block,
trappist_runtime::AssetId,
trappist_runtime::Balance,
trappist_runtime::AssetBalance,
>,
...
{
...
module.merge(Dex::new(client).into_rpc())?;
...
} Then the types are inferred in the following line from the service file: |
Trappist node refactor
As it was decided, we will refactor the node build setup, specifically over the
service.rs
+command.rs
files. In a nutshell we will get rid of the current runtime build featureswith-stout-runtime
andwith-trappist-runtime
and compile all native runtimes into one and only binarytrappist-collator
.Why we are not convinced with the current runtime build features setup?
chain_specs
: https://github.com/paritytech/trappist/blob/main/node/cli/src/command.rs#L65Refactor proposal
The proposal is to basically remove runtime build features and merge all the runtimes into one single collator file (
trappist-collator
). Also remove thecli
andservice
directories and comeback to thecommand.rs
andservice.rs
files, matching with the parachain template and common good parachains.There are some tweaks that need to be done on the service file like for example extending the types for the
start_node_impl
fn as:Also the
command.rs
need some additional code likeTODO: After we achieve this we can implement the build features with the right set up.
The text was updated successfully, but these errors were encountered: