Refactor common code among clients #316
Conversation
| @@ -115,6 +122,27 @@ thread_local!(static TIMESTAMP: std::cell::RefCell<u64> = std::cell::RefCell::ne | |||
| /// Provide a mock duration starting at 0 in millisecond for timestamp inherent. | |||
| /// Each call will increment timestamp by slot_duration making Aura think time has passed. | |||
| struct MockTimestampInherentDataProvider; | |||
There was a problem hiding this comment.
This can be moved to node_common as well
There was a problem hiding this comment.
It depends on the SLOT_DURATION in the runtimes, but we can probably be generic over it.
| /// Can only be called once on a `NodeBuilder` that doesn't have yet network | ||
| /// data. |
There was a problem hiding this comment.
I really like this, I knew it can be done using const generics and a state machine enum, but I didn't know about the TypeIdentity trick, will need to look into it.
There was a problem hiding this comment.
This is the "Typestate Pattern" described here: http://cliffle.com/blog/rust-typestate/
Usually you need an impl block per typestate, but core_extensions::TypeIdentity allows to state that a type parameter is equal to a concrete type and convert between both, which allow to have a single impl block with where clauses on the functions. (except for new, as it doesn't take self Rust is unable to infer which type to use, even if there is only 1 type T implementing TypeIdentity<Type = T>)
|
|
||
| let telemetry = telemetry.map(|(worker, telemetry)| { | ||
| task_manager | ||
| .spawn_handle() |
There was a problem hiding this comment.
Does not this spawn telemetry?
| /// Can only be called once on a `NodeBuilder` that doesn't have yet network | ||
| /// data. | ||
| #[must_use] | ||
| pub fn build_substrate_network( |
There was a problem hiding this comment.
So I cannot call together both build_substrate_network and build_cumulus_network right? I wonder whether this can be an enum as an argument, where in the case of type:
pub struct CumulusNetwork {
para_id: ParaId,
relay_chain_interface: RelayChainInterface
}
pub enum NetworkType {
Substrate,
Cumulus(CumulusNetwork)
}
There was a problem hiding this comment.
This enum would need to be generic over the RelayChainInterface type, which means that in Substrate mode it will not be able to infer the type and will require to explicitly name one.
Coverage Report@@ Coverage Diff @@
## master jeremy-refactor-clients +/- ##
===========================================================
- Coverage 71.37% 71.33% -0.04%
+ Files 86 89 +3
- Lines 19709 19592 -117
===========================================================
- Hits 14067 13974 -93
- Misses 5642 5618 -24
|
nanocryk
added
the
B5-clientnoteworthy
|
Left-over for future PRs:
|
|
Something in Edit: nevermind, it's failing on master as well. Will try to fix it and submit a pr. Edit2: #335 |
|
Can you resolve conflicts and merge? Thanks |
Adds a new crate which factors out common code between all clients, and allow to easily add node specific services.