Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document the process to launch your first (omni) node #3946

Closed
wants to merge 29 commits into from

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Apr 2, 2024

This PR finishes most of the groundwork needed for a tutorial from pallet -> runtime -> node.

Alongside it, edits a few other files to being them up to date. Most notably:

  • Finish wasm_meta_protocol ref doc with latest information about native runtime.
  • Move the pallet and the runtime code for the tutorial to its own crate, as this is needed to compile them to wasm.
  • Update templates page
  • Update some half finished pages with a better WIP message

Next:

  • Convert the same node to parachain
  • ref doc for custom runtime api and host function

@kianenigma kianenigma added the T11-documentation This PR/Issue is related to documentation. label Apr 2, 2024
@kianenigma kianenigma requested a review from a team as a code owner April 2, 2024 14:53
}

fn build_config(_config: Vec<u8>) -> apis::GenesisBuildResult {
// This is a temporary solution to set some initial state, please keep an eye on
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalkucharczyk this is only until your PR is merged.

docs/sdk/packages/guides/first-pallet/src/lib.rs Outdated Show resolved Hide resolved
use frame::prelude::*;

/// A trait meant to configure the pallet over types and values. See
/// [`frame::pallet_macros::config`] for more info.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth checking of linking via frame will take you to the useful docs from the frame_support re-export, or the useless final docs in frame_procedural. I think it would be the latter.

Suggested change
/// [`frame::pallet_macros::config`] for more info.
/// [`frame_support::pallet_macros::config`] for more info.

pub trait Config: frame_system::Config {}

/// The main struct that represents the functionality of the pallet. See
/// [`frame::pallet_macros::pallet`] for more info.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// [`frame::pallet_macros::pallet`] for more info.
/// [`frame_support::pallet_macros::pallet`] for more info.

docs/sdk/packages/guides/first-pallet/src/lib.rs Outdated Show resolved Hide resolved
// ensure that this is a signed account, but we don't really check `_anyone`.
let _anyone = ensure_signed(origin)?;

// update the balances map. Notice how all `<T: Config>` remains as `<T>`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what "Notice how all <T: Config> remains as <T>." means

@@ -1 +1,121 @@
//! # Your first Runtime
//!
//! This guide will walk you through the steps needed to add your pallet to an existing runtime.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! This guide will walk you through the steps needed to add your pallet to an existing runtime.
//! This guide will walk you through the steps to add your pallet to a runtime.

//! upgrades in [`crate::reference_docs::frame_runtime_upgrades_and_migrations`].
// TODO: explain spec better in its own doc and the ref doc on migration.
//!
//! Then, A real runtime also contains the `impl` of all individual pallets' `trait Config` for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! Then, A real runtime also contains the `impl` of all individual pallets' `trait Config` for
//! A real runtime also contains the `impl` of all individual pallets' `trait Config` for

Comment on lines +112 to +116
//! Historically, the node software also kept a native copy of the runtime at the time of
//! compilation within it. This used to be called the "Native Runtime". The main purpose of the
//! native runtime used to be leveraging the faster execution time and better debugging
//! infrastructure of native code. However, neither of the two arguments strongly hold and the
//! native runtime is being fully removed from the node-sdk.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! Historically, the node software also kept a native copy of the runtime at the time of
//! compilation within it. This used to be called the "Native Runtime". The main purpose of the
//! native runtime used to be leveraging the faster execution time and better debugging
//! infrastructure of native code. However, neither of the two arguments strongly hold and the
//! native runtime is being fully removed from the node-sdk.
//! Historically, the node also kept a native copy of the runtime. This was called the "Native Runtime". The main purpose of the
//! native runtime was leveraging the faster execution time and better debugging
//! infrastructure of native code. However, neither of the two arguments strongly hold and the
//! native runtime is being fully removed from the node-sdk.

//!
//! Therefore, it is utmost important to make sure before any runtime upgrade, the spec version is
//! updated.
// TODO: is this mentioned in the runtime upgrade ref doc?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mentioned very briefly here

//! Prior to building the new runtime, don't forget to update the
//! [`RuntimeVersion`](sp_version::RuntimeVersion).

Maybe the sp_version::RuntimeVersion docs should be made a bit richer, explaining more what it's used for?

/// Macro to amalgamate the runtime into `struct Runtime`.
// TODO: eventually change this to `use frame_support::construct_runtime as deprecated_construct_runtime;`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not instead mark it with #[deprecated]?

//! ```
//!
//! Or the equivalent from the tests of this crate:
#![doc = docify::embed!("./src/guides/your_first_node.rs", node_process)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manual seal parameter is in the code. Do we need it in bash one-liner?

//! For finality, there is one main option shipped with polkadot-sdk:
//!
//! * [`sc_consensus_grandpa`]/[`pallet_grandpa`]: A finality gadget that uses a voting mechanism to
//! decide when a block
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! decide when a block
//! decide when a block is finalized.

//! An omni-node is a new term in the polkadot-sdk (see
//! [here](https://github.com/paritytech/polkadot-sdk/pull/3597/) and
//! [here](https://github.com/paritytech/polkadot-sdk/issues/5)) and refers to a node that is
//! capable of running any runtime, so long as a certain set of assumptions are met. One of the most
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure:

Suggested change
//! capable of running any runtime, so long as a certain set of assumptions are met. One of the most
//! capable of running any runtime, as long as a certain set of assumptions are met. One of the most

Comment on lines +18 to +19
//! In the next section, we will use the latter approach for simplicity. [`your_first_runtime`] uses
//! has no consensus related code, and therefore can only be executed with a node that uses
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! In the next section, we will use the latter approach for simplicity. [`your_first_runtime`] uses
//! has no consensus related code, and therefore can only be executed with a node that uses
//! In the next section, we will use the latter approach for simplicity. [`your_first_runtime`]
//! has no consensus related code, and therefore can only be executed with a node that uses

//!
//! In the next section, we will use the latter approach for simplicity. [`your_first_runtime`] uses
//! has no consensus related code, and therefore can only be executed with a node that uses
//! [`sc_consensus_manual_seal`], namely `minima-omni-node`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! [`sc_consensus_manual_seal`], namely `minima-omni-node`.
//! [`sc_consensus_manual_seal`], namely `minimal-omni-node`.

//! The only detail that we will cover for now is that to enable an easy way for the runtime to
//! generate some initial state, as it greatly makes the development process easier. For the time
//! being, please pay attention to the implementation of [`build_config`] in the corresponding
//! `Runtime`. For now, we always populate the chian with some funds in all of the accounts named in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! `Runtime`. For now, we always populate the chian with some funds in all of the accounts named in
//! `Runtime`. For now, we always populate the chain with some funds in all of the accounts named in

/// Please note that provided JSON blob must contain all `RuntimeGenesisConfig` fields, no
/// defaults will be used.
fn build_state(json: Vec<u8>) -> Result;
pub mod runtime_api {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better to move to the runtime_api.rs file?
Avoids indentation, also follows the way how it is done in some of other crates.

Why do we need it after all? I see some crates not having this namespace.

//! Historically, the node software also kept a native copy of the runtime at the time of
//! compilation within it. This used to be called the "Native Runtime". The main purpose of the
//! native runtime used to be leveraging the faster execution time and better debugging
//! infrastructure of native code. However, neither of the two arguments strongly hold and the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Is really debugging wasm runtime as easy as native one?
I would maybe mention the huge maintenance burden of native execution path, and possibly nondeterminism of native vs wasm e.g. when compile with different compilers. Not sure if we want to get into this here.

#[docify::export]
/// Single storage item, of type `Balance`.
#[pallet::storage]
pub type TotalIssuance<T: Config> = StorageValue<Value = Balance>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its good practice to always put an OptionQuery or ValueQuery explicitly here, so that new devs are aware.
I often see in SE questions that they dont realize that OptionQuery even exists 🙈

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question if such doc should have more or less details like that. I also see that call indexes are not setup explicitly. In this particular case I would pass the setting explicitly.

#[docify::export]
/// Single storage item, of type `Balance`.
#[pallet::storage]
pub type TotalIssuance<T: Config> = StorageValue<Value = Balance>;
Copy link
Member

@ggwpez ggwpez Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its good practice to always put an OptionQuery or ValueQuery here, so that new devs are aware.
I often see in SE questions that they dont realize that OptionQuery even exists 🙈

let _anyone = ensure_signed(origin)?;

// update the balances map. Notice how all `<T: Config>` remains as `<T>`.
Balances::<T>::mutate(dest, |b| *b = Some(b.unwrap_or(0) + amount));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not overflow safe

// update the balances map. Notice how all `<T: Config>` remains as `<T>`.
Balances::<T>::mutate(dest, |b| *b = Some(b.unwrap_or(0) + amount));
// update total issuance.
TotalIssuance::<T>::mutate(|t| *t = Some(t.unwrap_or(0) + amount));
Copy link
Member

@ggwpez ggwpez Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto. Ppl will copy paste this 🙈
Ok nvm i should read it in the context of the guide.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to check it out in the browser but does currently not build.

}

/// Transfer `amount` from `origin` to `dest`.
pub fn transfer(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn transfer(
pub fn transfer_bad_practice(

or something like this?

let sender_balance = Balances::<T>::get(&sender).ok_or("NonExistentAccount")?;
let reminder = sender_balance.checked_sub(amount).ok_or("InsufficientBalance")?;

// .. snip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should also check the Balances and TI additions.

let reminder =
sender_balance.checked_sub(amount).ok_or(Error::<T>::InsufficientBalance)?;

Balances::<T>::mutate(&dest, |b| *b = Some(b.unwrap_or(0) + amount));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not overflow safe

StateBuilder::default().build_and_execute(|| {
// skip the genesis block, as events are not deposited there and we need them for
// the final assertion.
System::set_block_number(ALICE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
System::set_block_number(ALICE);
System::set_block_number(1);

Alice? I dont see the connection here.

//! * [`your_first_runtime`], where you learn how to compile your pallets into a WASM runtime.
//! * [`your_first_node`], where you learn how to run a node with your runtime.
//!
//! > By this step, you have already launched a full blockchain!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds nice, but the pallets do have dev_mode. Could be that people miss that and think they are ready for launch...

//! The only detail that we will cover for now is that to enable an easy way for the runtime to
//! generate some initial state, as it greatly makes the development process easier. For the time
//! being, please pay attention to the implementation of [`build_config`] in the corresponding
//! `Runtime`. For now, we always populate the chian with some funds in all of the accounts named in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! `Runtime`. For now, we always populate the chian with some funds in all of the accounts named in
//! `Runtime`. For now, we always populate the chain with some funds in all of the accounts named in

//! [`build_config`]: polkadot_sdk_docs_packages_guides_first_runtime::Runtime#method.build_config

#[test]
#[ignore = "will not work until we have good omni-nodes in this repo; wait for #3597"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[ignore = "will not work until we have good omni-nodes in this repo; wait for #3597"]
#[cfg(feature = "will not work until we have good omni-nodes in this repo; wait for #3597")]

I think CI runs with --ignored.

//!
//! ## Setup
//!
//! A pallet is typically a normal rust-crate with the following properties:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! A pallet is typically a normal rust-crate with the following properties:
//! A pallet is typically a normal Rust-crate with the following properties:

ensure!(sender_balance >= amount, "InsufficientBalance");
let reminder = sender_balance - amount;

// .. snip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Balances amount addition is not using safe math.

Comment on lines +160 to +168
construct_runtime!(
pub struct Runtime {
// ---^^^^^^ This is where `struct Runtime` is defined.
System: frame_system,
Currency: pallet_currency,
// this macro expects us to express pallets as "<some name for pallet>: <path
// to pallet>"
}
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still a bit experimental as we are just testing it on Rococo+Westend. But could still be mentioned i guess.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team April 26, 2024 12:41
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6069019

Copy link
Contributor

@muharem muharem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we eventually want to have this on docs.rs?

I really like these new style docs; they are more story-driven and engineer-oriented. However, I find it quite hard to navigate through our documentations. I see at least two problems.

First, I usually can't find the right doc page through Google. I only find it by navigating to a specific source because I know it should be there somewhere. Also, in the search results, you often see several different sources (substrate/polkadot/docs.rs), which can be confusing.

Second, we have many learning resources for our libraries, tools, and even for some general crypto topics. I think an engineer might not need to learn all of it, especially when they are just getting started. We might need to create an index page where we can direct engineers with different goals to appropriate guidelines or provide recommendations on what to focus on initially and what they can learn later (or they wont even need).

I recently read the documentation of a popular L1 solution. I went to the official website, navigated to the documentation page, and read through all of it in a few hours. I realized that you can't simply do the same with our docs due to our broader domain and the way they are spread across different web resources. While we can't necessarily shorten our documentations, we can try to make it easier to navigate and highlight what you really need to focus on to achieve your goals.

#[docify::export]
/// Single storage item, of type `Balance`.
#[pallet::storage]
pub type TotalIssuance<T: Config> = StorageValue<Value = Balance>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question if such doc should have more or less details like that. I also see that call indexes are not setup explicitly. In this particular case I would pass the setting explicitly.

pub(super) type SignedExtra = (
// `frame` already provides all the signed extensions from `frame-system`. We just add the
// one related to tx-payment here.
frame::runtime::types_common::SystemSignedExtensionsOf<Runtime>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does not include new metadata hash extension

type RuntimeEvent = RuntimeEvent;
}

impl_runtime_apis! {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a short doc with a link to the full doc would be helpful here.

@kianenigma
Copy link
Contributor Author

First, I usually can't find the right doc page through Google. I only find it by navigating to a specific source because I know it should be there somewhere.

I genrally have the same issue. Once we publish this to docs.rs, it would be better, but end of the day a SEO optimized website is probably outside the scope.

Second, we have many learning resources for our libraries, tools, and even for some general crypto topics.

Maybe #4650 is what you are asking for?

I recently read the documentation of a popular L1 solution

It is worth noting that the starting goal of the polkadot-sdk-docs was not to compete with a fancy high level doc portal. It is meant to be low level, always correct and crude body of information to enable others to build on top of it. It is meant to educate the educators, and be the backbone of information.

Does this make sense?

and with this goal in mind, I care less atm about something like SEO optimization or the CSS of the pages. A few DF teams are working on better high level devhubs and I am sure they will do a better job than we core rust devs can do in making websites :)

@kianenigma
Copy link
Contributor Author

BTW this PR is stale and should be closed, I will re-open it part by part later.

@kianenigma kianenigma closed this Jul 23, 2024
@kianenigma kianenigma reopened this Jul 23, 2024
@paritytech-review-bot paritytech-review-bot bot requested a review from a team July 23, 2024 09:49
@kianenigma kianenigma mentioned this pull request Jul 23, 2024
@kianenigma
Copy link
Contributor Author

kianenigma commented Oct 16, 2024

FINALLY closed with #6094

@kianenigma kianenigma closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T11-documentation This PR/Issue is related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants