Madara coding best practices

[ClementWalter]

Context

I'm new to rust and madara both at the same time.

Goal

I am describing here some misunderstandings or patterns that confused me when entering the madara repo. This discovery report could serve as a base for defining madara best practices to smoothen newcomers introduction.

Topics

Imports

I find it very confusing to understand where the things are imported from or defined (unlike python or typescript for example). I’m always asking myself « where is this defined » and « where is this used ».

=> should the star import should be ban (not a good practices in the languages I’ve been used to, not sure about rust) ?
=> even though valid, should we enforce as much as possible the definition of functions/objects in a progressive order (as in interpreted languages) ?

Naming

• the mock runtime is simply called Test
• variable names suffixed by their type (e.g. my_var_str) instead of shadowing
• name and type clash, e.g. const MY_CONTRACT_PATH: &[u8] = include_bytes!("../path/to/contract.json"); ie name says it's a path (one expects either a &str or a dedicate type for filesystem path, but it's a file content)

Tests

Hard-coded values mingled into the code

• mock.rs
• chain_spec.rs

Could be easier to move all config/constant declaration into a static file (json/toml or even rust, but with no logic, just const)

Types

ContractClass

Blockifier defines a struct ContractClass as follows

pub struct ContractClass {
    pub program: starknet_api::deprecated_contract_class::Program,
    /// The selector of each entry point is a unique identifier in the program.
    pub entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>,
    // Not required for execution, thus can be omitted from the raw contract file.
    pub abi: Option<serde_json::Value>,
}

and it appears (at least in tests) that we also need the class hash, which could probably be derived from these information. Instead, we have another variable storing only the hash, creating both confusion and a risk for discrepancy

We also have a

#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))]
pub struct ContractClassWrapper {
    /// Contract class program json.
    pub program: BoundedVec<u8, MaxProgramSize>,
    // /// Contract class abi.
    // pub abi: BoundedVec<ContractClassAbiEntryWrapper, MaxAbiSize>,
    /// Contract class entrypoints.
    pub entry_points_by_type: BoundedVec<u8, MaxEntryPoints>,
}

with a commented attribute (abi). In some other places with also have commented code blocks, for example

Definition

Types seem to be defined at different places of the code, whenever it was probably required to have them, which prevents the newcomer (me) from having a clear vision of the types used in the project, with all types being grouped at the same place.

Preludes

We have one extern crate frame_benchmarking; which may not be idiomatic from rust 2018 on.

Miscellanous

Code ordering

Even though no ordering block declaration is enforced by rust (like defining the struct before its impl), I'd find it easier to avoid as much as possible to use yet non defined variables so as to ease the reading of a file (we read from top to bottom, unlike rust).

For example, this is confusing to me (see chain_spec.rs):

pub type DevChainSpec = sc_service::GenericChainSpec<DevGenesisExt>;

/// Extension for the dev genesis config to support a custom changes to the genesis state.
#[derive(Serialize, Deserialize)]
pub struct DevGenesisExt {
    /// Genesis config.
    genesis_config: GenesisConfig,
    /// The flag that if enable manual-seal mode.
    enable_manual_seal: Option<bool>,
}

because I'm facing an unknown DevGenesisExt (not defined from top to pub type), need to think about it, where does it come from, eventually resign, to realize that it's defined just below.

Error message

Just found this

TPublic::Pair::from_string(&format!("//{seed}"), None).expect("static values are valid; qed")

and the error message doesn't help me much