The following is a list of opportunities for improvement in substrate. If you personally contribute to substrate, this may be difficult to read and difficilt to accept. It's constructive critisism. You'll be tempted to ignore it, but please to consider the points, it's for the good of substrate and substrate users.
Actual line from generated docs:
pub trait Trait: Trait {
...
}From what I can tell, SMRL uses module_name::Trait to convey a special meaning. I suggest not doing this.
"Trait" aleady has a meaning in Rust.
Consider a user trying to learn substrate. She sees the line: pub trait Trait: system::Trait { and wonders
what system::Trait does. She searches for
Trait, finds the
docs for smrl_system::Trait out
to the list of 19 distinct smrl traits named "Trait". As it turns out
smrl_system::Trait is not documented.
At this point the user gives up and decides to write a feedback document instread of making progress
using substrate. This is not a hypothetical.
Rust conventions is to attach documentation to the entity in question (smrl_system::Trait), not
just the module (smrl_system) containing the entity.
Macros have costs.
- Macros are not self documenting. Users need to read manually written documentation or infer usage from examples. If no such documentation exists, to learn the api of a macro the user must read and understand the macro's source code and macro source tends to be pretty inscrutable in Rust.
- Macros don't produce readable error messages.
- Macros make type-checking hard to understand.
- Rustfmt does not understand most macro calls.
Macros are commonly used in substrate to introduce new DSLs. The users needs to learn those DLS (sometimes in addition to learning Rust) before they can use substrate.
Some, hopefully most, of the macros in substrate can be impelented as non-macros.
For example:
construct_simple_protocol! {
pub struct MyProtocol where Block = MyBlock {
consensus_gossip: ConsensusGossip<MyBlock>,
other_protocol: MyCoolStuff,
}
}should be replaced with:
pub type MyProtocol = specialization::NetworkComposite<ConsensusGossip<MyBlock>, MyCoolStuff>;or:
#[derive(specialization::NetworkSpecialization)]
pub struct MyProtocol {
consensus_gossip: ConsensusGossip<MyBlock>,
other_protocol: MyCoolStuff,
}
// I know, derive is still a macro, but derive semantics are well known and therefore not as costly
// to users.It seems most, if not all, substrate macros are declarative and they take imitatations of rust syntax as input. Declarative macros are brittle when used this way. If you want to take Rust syntax as input, don't implement your own custom parser; use the syn crate.
"But custom declarative parsers allow us to extend struct declaration syntax with 'where' clauses."
Yes, they do. Don't use custom 'where' clauses. It's confusing to beginners. That temptation to be clever is a trap. When you fall for that trap you make the project more complex.
What does smrl_system mean? It's not immediately clear.
I get that naming things is hard. If you come up with more precise name for smrl_system it will
help users understand how to use it. You may find that naming things forces you to understand what
those things are actually for.
Overly generic names are common throughout substrate, TODO: expand
4 - Read up on impl Trait syntax.
The newish syntax, stablized in rust 1.26, allows for simplified generic function definitions. In argument position, the new syntax aids in reduction of template parameter variables, useful in places like the parse_and_execute function.
For example:
pub fn run<I, T, E>(args: I, exit: E, version: VersionInfo) -> error::Result<()> where
I: IntoIterator<Item = T>,
T: Into<std::ffi::OsString> + Clone,
E: IntoExit,
{
...
}becomes
pub fn run(
args: impl Iterator<Item = impl Into<std::ffi::OsString> + Clone>,
exit: impl IntoExit,
version: VersionInfo,
) -> error::Result<()> {
...
}This isn't about aesthetics, it's about readability and modifiability. The above example removes one layer of indirection for the reader, thereby reducing cognitive load.
In some cases named template parameters are still a better fit. It's useful to know both syntaxes.
When substrate runtimes check for "std" with #[cfg(feature = "std")], the intent is actually to
check whether the code is being compiled to wasm. Be explicit about this.
Ideally, naitive and wasm runtimes compile from the same code, no conditional compilation on
#[cfg(feature = "std")]. It should be possible to refactor for isomorphism.
From substrate docs
// - NO SIDE-EFFECTS ON ERROR: This function must either complete totally (and return
// `Ok(())` or it must have no side-effects on storage and return `Err('Some reason')`.
This gives the user an opportunity to mess things up in nasty ways. It's never valid for the user to write state changes, then return an error, so it should me made impossible. I can think of two ways to prevent side effects on error.
fn buy_gas(origin, count: u64) -> Result {
const cost_to_run: u64 = 5;
let sender = ensure_signed(origin)?;
ensure!(
get_gas_balance(sender)?.checked_add(count)? >= cost_to_run,
"not enough gas to buy gas"
);
decrease_token_balance(sender, count)?;
increase_gas_balance(sender, count).expect("gas overflow checked above");
pay_gas(sender, cost_to_run).expect("gas underflow checked above");
Ok(())
}Option 1. Let the user explicitly return as set of changes.
fn buy_gas(origin, count: u64) -> Result<Changes> {
const cost_to_run: u64 = 5;
let sender = ensure_signed(origin)?;
let mut changes = Changes::new();
changes.insert(
Balances::token_balance(sender),
get(Balances::token_balance(sender))?.checked_sub(count)?
);
changes.insert(
Gas::gas_balance(sender),
get(Balances::gas(sender))?.checked_add(count)?.checked_sub(cost_to_run)?
);
Ok(changes)
}Option 2. Implicitly store changes, only applying them when the the user function returns Ok(()).
fn buy_gas(origin, count: u64) -> Result {
const cost_to_run: u64 = 5;
let sender = ensure_signed(origin)?;
decrease_token_balance(sender, count)?;
increase_gas_balance(sender, count)?;
pay_gas(sender, cost_to_run)?;
Ok(())
}Update: https://github.com/paritytech/substrate/issues/2980 paritytech/substrate#3263 Horray!
angry rant
For fuck sake, it's 2019, use an auto-formatter."But bddap, rustfmt doesn't support in our bespoke style guide." In that case, your style guide is a detriment to productivity. Probably >5% of your contributors' keyboard time is spent making their code look just so. There are probably formatting comments as feedback on your prs. Manual code formating is a waste of time, it makes contributors unhappy. Code formatting is a solved problem, you are wasting your own time, the time of Parity employees, substrate contrubutors' time.
Ok, sorry. Parity's aversion of autoformatters has bothered me for a long time.
tl;dr I highly recommend trying rustfmt, its super nice.