From b56aed1c4bc7869196f21f231a62f940e143c609 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 5 Dec 2023 17:41:04 +0100 Subject: [PATCH 1/3] improve polkadot sdk docs --- docs/sdk/Cargo.toml | 2 +- docs/sdk/src/guides/mod.rs | 2 +- docs/sdk/src/guides/your_first_pallet/mod.rs | 131 ++++++++++-------- .../src/reference_docs/fee_less_runtime.rs | 2 +- 4 files changed, 76 insertions(+), 61 deletions(-) diff --git a/docs/sdk/Cargo.toml b/docs/sdk/Cargo.toml index f5bfd80fd10d..14b4747d558d 100644 --- a/docs/sdk/Cargo.toml +++ b/docs/sdk/Cargo.toml @@ -22,7 +22,7 @@ pallet-default-config-example = { path = "../../substrate/frame/examples/default simple-mermaid = { git = "https://github.com/kianenigma/simple-mermaid.git", branch = "main" } docify = "0.2.6" -# Polkadot SDK deps, typically all should only be scope such that we can link to their doc item. +# Polkadot SDK deps, typically all should only be in scope such that we can link to their doc item. node-cli = { package = "staging-node-cli", path = "../../substrate/bin/node/cli" } kitchensink-runtime = { path = "../../substrate/bin/node/runtime" } chain-spec-builder = { package = "staging-chain-spec-builder", path = "../../substrate/bin/utils/chain-spec-builder" } diff --git a/docs/sdk/src/guides/mod.rs b/docs/sdk/src/guides/mod.rs index 68ca9317b907..3120f2533109 100644 --- a/docs/sdk/src/guides/mod.rs +++ b/docs/sdk/src/guides/mod.rs @@ -1,7 +1,7 @@ //! # Polkadot SDK Docs Guides //! //! This crate contains a collection of guides that are foundational to the developers of -//! Polkadot SDK. They common user-journeys that are traversed in the Polkadot ecosystem. +//! Polkadot SDK. They are common user-journeys that are traversed in the Polkadot ecosystem. /// Write your first simple pallet, learning the most most basic features of FRAME along the way. pub mod your_first_pallet; diff --git a/docs/sdk/src/guides/your_first_pallet/mod.rs b/docs/sdk/src/guides/your_first_pallet/mod.rs index 855b390aba9d..9a8772ec582c 100644 --- a/docs/sdk/src/guides/your_first_pallet/mod.rs +++ b/docs/sdk/src/guides/your_first_pallet/mod.rs @@ -12,12 +12,11 @@ //! > This guide will build a currency pallet from scratch using only the lowest primitives of //! > FRAME, and is mainly intended for education, not *applicability*. For example, almost all //! > FRAME-based runtimes use various techniques to re-use a currency pallet instead of writing -//! > one. Further advance FRAME related topics are discussed in [`crate::reference_docs`]. +//! > one. Further advanced FRAME related topics are discussed in [`crate::reference_docs`]. //! //! ## Topics Covered //! -//! The following FRAME topics are covered in this guide. See the documentation of the -//! associated items to know more. +//! The following FRAME topics are covered in this guide: //! //! - [Storage](frame::pallet_macros::storage) //! - [Call](frame::pallet_macros::call) @@ -50,8 +49,10 @@ //! //! One should be a mapping from account-ids to a balance type, and one value that is the total //! issuance. -// -// For the rest of this guide, we will opt for a balance type of u128. +//! +//! > For the rest of this guide, we will opt for a balance type of u128. For the sake of +//! > simplicity, we are hardcoding this type. In a real pallet is best practice to define it as a +//! > generic bounded type in the `Config` trait, and then specify it in the implementation. #![doc = docify::embed!("./src/guides/your_first_pallet/mod.rs", Balance)] //! //! The definition of these two storage items, based on [`frame::pallet_macros::storage`] details, @@ -160,13 +161,12 @@ #![doc = docify::embed!("./src/guides/your_first_pallet/mod.rs", first_test)] //! //! In the first test, we simply assert that there is no total issuance, and no balance associated -//! with account `1`. Then, we mint some balance into `1`, and re-check. +//! with ALICE's account. Then, we mint some balance into ALICE's, and re-check. //! //! As noted above, the `T::AccountId` is now `u64`. Moreover, `Runtime` is replacing ``. //! This is why for example you see `Balances::::get(..)`. Finally, notice that the //! dispatchables are simply functions that can be called on top of the `Pallet` struct. -//! -//! TODO: hard to explain exactly `RuntimeOrigin::signed(1)` at this point. +// TODO: hard to explain exactly `RuntimeOrigin::signed(ALICE)` at this point. //! //! Congratulations! You have written your first pallet and tested it! Next, we learn a few optional //! steps to improve our pallet. @@ -186,8 +186,8 @@ #![doc = docify::embed!("./src/guides/your_first_pallet/mod.rs", StateBuilder)] //! //! This struct is meant to contain the same list of accounts and balances that we want to have at -//! the beginning of each block. We hardcoded this to `let accounts = vec![(1, 100), (2, 100)];` so -//! far. Then, if desired, we attach a default value for this struct. +//! the beginning of each block. We hardcoded this to `let accounts = vec![(ALICE, 100), (2, 100)];` +//! so far. Then, if desired, we attach a default value for this struct. #![doc = docify::embed!("./src/guides/your_first_pallet/mod.rs", default_state_builder)] //! //! Like any other builder pattern, we attach functions to the type to mutate its internal @@ -222,7 +222,7 @@ //! "success path" of a dispatchable, and one test for each "failure path", such as: #![doc = docify::embed!("./src/guides/your_first_pallet/mod.rs", transfer_from_non_existent_fails)] //! -//! We leave it up to you to write a test that triggers to `InsufficientBalance` error. +//! We leave it up to you to write a test that triggers the `InsufficientBalance` error. //! //! ### Event and Error //! @@ -230,21 +230,23 @@ //! Errors. First, let's understand what each is. //! //! - **Error**: The static string-based error scheme we used so far is good for readability, but it -//! has a few drawbacks. These string literals will bloat the final wasm blob, and are relatively -//! heavy to transmit and encode/decode. Moreover, it is easy to mistype them by one character. -//! FRAME errors are exactly a solution to maintain readability, whilst fixing the drawbacks -//! mentioned. In short, we use an enum to represent different variants of our error. These -//! variants are then mapped in an efficient way (using only `u8` indices) to +//! has a few drawbacks. The biggest problem with strings are that they are not type safe, e.g. a +//! match statement cannot be exhaustive. These string literals will bloat the final wasm blob, +//! and are relatively heavy to transmit and encode/decode. Moreover, it is easy to mistype them +//! by one character. FRAME errors are exactly a solution to maintain readability, whilst fixing +//! the drawbacks mentioned. In short, we use an enum to represent different variants of our +//! error. These variants are then mapped in an efficient way (using only `u8` indices) to //! [`sp_runtime::DispatchError::Module`] Read more about this in [`frame::pallet_macros::error`]. //! -//! - **Event**: Events are akin to the return type of dispatchables. They should represent what -//! happened at the end of a dispatch operation. Therefore, the convention is to use passive tense -//! for event names (eg. `SomethingHappened`). This allows other sub-systems or external parties -//! (eg. a light-node, a DApp) to listen to particular events happening, without needing to -//! re-execute the whole state transition function. -//! -//! TODO: both need to be improved a lot at the pallet-macro rust-doc level. Also my explanation -//! of event is probably not the best. +//! - **Event**: Events are akin to the return type of dispatchables. They are mostly data blobs +//! emitted by the runtime to let outside world know what is happening inside the pallet. Since +//! otherwise, the outside world does not have an easy access to the state changes. They should +//! represent what happened at the end of a dispatch operation. Therefore, the convention is to +//! use passive tense for event names (eg. `SomethingHappened`). This allows other sub-systems or +//! external parties (eg. a light-node, a DApp) to listen to particular events happening, without +//! needing to re-execute the whole state transition function. +// TODO: both need to be improved a lot at the pallet-macro rust-doc level. Also my explanation +// of event is probably not the best. //! //! With the explanation out of the way, let's see how these components can be added. Both follow a //! fairly familiar syntax: normal Rust enums, with an extra `#[frame::event/error]` attribute @@ -413,6 +415,9 @@ pub mod pallet { pub(crate) mod tests { use crate::guides::your_first_pallet::pallet::*; use frame::testing_prelude::*; + const ALICE: u64 = 1; + const BOB: u64 = 2; + const CHARLIE: u64 = 3; #[docify::export] mod runtime { @@ -447,7 +452,7 @@ pub mod pallet { #[docify::export] fn new_test_state_basic() -> TestState { let mut state = TestState::new_empty(); - let accounts = vec![(1, 100), (2, 100)]; + let accounts = vec![(ALICE, 100), (BOB, 100)]; state.execute_with(|| { for (who, amount) in &accounts { Balances::::insert(who, amount); @@ -466,7 +471,7 @@ pub mod pallet { #[docify::export(default_state_builder)] impl Default for StateBuilder { fn default() -> Self { - Self { balances: vec![(1, 100), (2, 100)] } + Self { balances: vec![(ALICE, 100), (BOB, 100)] } } } @@ -509,15 +514,19 @@ pub mod pallet { #[test] fn first_test() { TestState::new_empty().execute_with(|| { - // We expect account 1 to have no funds. - assert_eq!(Balances::::get(&1), None); + // We expect ALICE's account to have no funds. + assert_eq!(Balances::::get(&ALICE), None); assert_eq!(TotalIssuance::::get(), None); - // mint some funds into 1 - assert_ok!(Pallet::::mint_unsafe(RuntimeOrigin::signed(1), 1, 100)); + // mint some funds into ALICE's + assert_ok!(Pallet::::mint_unsafe( + RuntimeOrigin::signed(ALICE), + ALICE, + 100 + )); // re-check the above - assert_eq!(Balances::::get(&1), Some(100)); + assert_eq!(Balances::::get(&ALICE), Some(100)); assert_eq!(TotalIssuance::::get(), Some(100)); }) } @@ -526,9 +535,9 @@ pub mod pallet { #[test] fn state_builder_works() { StateBuilder::default().build_and_execute(|| { - assert_eq!(Balances::::get(&1), Some(100)); - assert_eq!(Balances::::get(&2), Some(100)); - assert_eq!(Balances::::get(&3), None); + assert_eq!(Balances::::get(&ALICE), Some(100)); + assert_eq!(Balances::::get(&BOB), Some(100)); + assert_eq!(Balances::::get(&CHARLIE), None); assert_eq!(TotalIssuance::::get(), Some(200)); }); } @@ -536,8 +545,8 @@ pub mod pallet { #[docify::export] #[test] fn state_builder_add_balance() { - StateBuilder::default().add_balance(3, 42).build_and_execute(|| { - assert_eq!(Balances::::get(&3), Some(42)); + StateBuilder::default().add_balance(CHARLIE, 42).build_and_execute(|| { + assert_eq!(Balances::::get(&CHARLIE), Some(42)); assert_eq!(TotalIssuance::::get(), Some(242)); }) } @@ -546,10 +555,10 @@ pub mod pallet { #[should_panic] fn state_builder_duplicate_genesis_fails() { StateBuilder::default() - .add_balance(3, 42) - .add_balance(3, 43) + .add_balance(CHARLIE, 42) + .add_balance(CHARLIE, 43) .build_and_execute(|| { - assert_eq!(Balances::::get(&3), None); + assert_eq!(Balances::::get(&CHARLIE), None); assert_eq!(TotalIssuance::::get(), Some(242)); }) } @@ -559,17 +568,21 @@ pub mod pallet { fn mint_works() { StateBuilder::default().build_and_execute(|| { // given the initial state, when: - assert_ok!(Pallet::::mint_unsafe(RuntimeOrigin::signed(1), 2, 100)); + assert_ok!(Pallet::::mint_unsafe(RuntimeOrigin::signed(ALICE), BOB, 100)); // then: - assert_eq!(Balances::::get(&2), Some(200)); + assert_eq!(Balances::::get(&BOB), Some(200)); assert_eq!(TotalIssuance::::get(), Some(300)); // given: - assert_ok!(Pallet::::mint_unsafe(RuntimeOrigin::signed(1), 3, 100)); + assert_ok!(Pallet::::mint_unsafe( + RuntimeOrigin::signed(ALICE), + CHARLIE, + 100 + )); // then: - assert_eq!(Balances::::get(&3), Some(100)); + assert_eq!(Balances::::get(&CHARLIE), Some(100)); assert_eq!(TotalIssuance::::get(), Some(400)); }); } @@ -579,19 +592,19 @@ pub mod pallet { fn transfer_works() { StateBuilder::default().build_and_execute(|| { // given the the initial state, when: - assert_ok!(Pallet::::transfer(RuntimeOrigin::signed(1), 2, 50)); + assert_ok!(Pallet::::transfer(RuntimeOrigin::signed(ALICE), BOB, 50)); // then: - assert_eq!(Balances::::get(&1), Some(50)); - assert_eq!(Balances::::get(&2), Some(150)); + assert_eq!(Balances::::get(&ALICE), Some(50)); + assert_eq!(Balances::::get(&BOB), Some(150)); assert_eq!(TotalIssuance::::get(), Some(200)); // when: - assert_ok!(Pallet::::transfer(RuntimeOrigin::signed(2), 1, 50)); + assert_ok!(Pallet::::transfer(RuntimeOrigin::signed(BOB), ALICE, 50)); // then: - assert_eq!(Balances::::get(&1), Some(100)); - assert_eq!(Balances::::get(&2), Some(100)); + assert_eq!(Balances::::get(&ALICE), Some(100)); + assert_eq!(Balances::::get(&BOB), Some(100)); assert_eq!(TotalIssuance::::get(), Some(200)); }); } @@ -602,14 +615,14 @@ pub mod pallet { StateBuilder::default().build_and_execute(|| { // given the the initial state, when: assert_err!( - Pallet::::transfer(RuntimeOrigin::signed(3), 1, 10), + Pallet::::transfer(RuntimeOrigin::signed(CHARLIE), ALICE, 10), "NonExistentAccount" ); // then nothing has changed. - assert_eq!(Balances::::get(&1), Some(100)); - assert_eq!(Balances::::get(&2), Some(100)); - assert_eq!(Balances::::get(&3), None); + assert_eq!(Balances::::get(&ALICE), Some(100)); + assert_eq!(Balances::::get(&BOB), Some(100)); + assert_eq!(Balances::::get(&CHARLIE), None); assert_eq!(TotalIssuance::::get(), Some(200)); }); } @@ -685,6 +698,8 @@ pub mod pallet_v2 { pub mod tests { use super::{super::pallet::tests::StateBuilder, *}; use frame::testing_prelude::*; + const ALICE: u64 = 1; + const BOB: u64 = 2; #[docify::export] pub mod runtime_v2 { @@ -717,20 +732,20 @@ pub mod pallet_v2 { 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(1); + System::set_block_number(ALICE); // given the the initial state, when: - assert_ok!(Pallet::::transfer(RuntimeOrigin::signed(1), 2, 50)); + assert_ok!(Pallet::::transfer(RuntimeOrigin::signed(ALICE), BOB, 50)); // then: - assert_eq!(Balances::::get(&1), Some(50)); - assert_eq!(Balances::::get(&2), Some(150)); + assert_eq!(Balances::::get(&ALICE), Some(50)); + assert_eq!(Balances::::get(&BOB), Some(150)); assert_eq!(TotalIssuance::::get(), Some(200)); // now we can also check that an event has been deposited: assert_eq!( System::read_events_for_pallet::>(), - vec![Event::Transferred { from: 1, to: 2, amount: 50 }] + vec![Event::Transferred { from: ALICE, to: BOB, amount: 50 }] ); }); } diff --git a/docs/sdk/src/reference_docs/fee_less_runtime.rs b/docs/sdk/src/reference_docs/fee_less_runtime.rs index 43a761a6c52c..1213c2628253 100644 --- a/docs/sdk/src/reference_docs/fee_less_runtime.rs +++ b/docs/sdk/src/reference_docs/fee_less_runtime.rs @@ -9,4 +9,4 @@ //! and some kind of rate limiting (eg. any account gets 5 free tx per day). //! - The rule of thumb is that as long as the unsigned validate does one storage read, similar to //! nonce, it is fine. -//! - This could possibly be a good tutorial/template, rather than a reference doc. +//! - This could possibly be a good guide/template, rather than a reference doc. From 2ff45b134fc9691929de6ce46595beba8e5d8ed7 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Wed, 6 Dec 2023 14:17:31 +0100 Subject: [PATCH 2/3] reivew comments --- docs/sdk/src/guides/your_first_pallet/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/sdk/src/guides/your_first_pallet/mod.rs b/docs/sdk/src/guides/your_first_pallet/mod.rs index 9a8772ec582c..1134d36549c9 100644 --- a/docs/sdk/src/guides/your_first_pallet/mod.rs +++ b/docs/sdk/src/guides/your_first_pallet/mod.rs @@ -50,7 +50,7 @@ //! One should be a mapping from account-ids to a balance type, and one value that is the total //! issuance. //! -//! > For the rest of this guide, we will opt for a balance type of u128. For the sake of +//! > For the rest of this guide, we will opt for a balance type of `u128`. For the sake of //! > simplicity, we are hardcoding this type. In a real pallet is best practice to define it as a //! > generic bounded type in the `Config` trait, and then specify it in the implementation. #![doc = docify::embed!("./src/guides/your_first_pallet/mod.rs", Balance)] @@ -161,7 +161,7 @@ #![doc = docify::embed!("./src/guides/your_first_pallet/mod.rs", first_test)] //! //! In the first test, we simply assert that there is no total issuance, and no balance associated -//! with ALICE's account. Then, we mint some balance into ALICE's, and re-check. +//! with Alice's account. Then, we mint some balance into Alice's, and re-check. //! //! As noted above, the `T::AccountId` is now `u64`. Moreover, `Runtime` is replacing ``. //! This is why for example you see `Balances::::get(..)`. Finally, notice that the @@ -514,11 +514,11 @@ pub mod pallet { #[test] fn first_test() { TestState::new_empty().execute_with(|| { - // We expect ALICE's account to have no funds. + // We expect Alice's account to have no funds. assert_eq!(Balances::::get(&ALICE), None); assert_eq!(TotalIssuance::::get(), None); - // mint some funds into ALICE's + // mint some funds into Alice's account. assert_ok!(Pallet::::mint_unsafe( RuntimeOrigin::signed(ALICE), ALICE, From 63c266f063952b7fc6993e93100174941c2f33ca Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Wed, 6 Dec 2023 14:19:07 +0100 Subject: [PATCH 3/3] reivew comments --- docs/sdk/src/guides/your_first_pallet/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/sdk/src/guides/your_first_pallet/mod.rs b/docs/sdk/src/guides/your_first_pallet/mod.rs index 1134d36549c9..c886bc9af842 100644 --- a/docs/sdk/src/guides/your_first_pallet/mod.rs +++ b/docs/sdk/src/guides/your_first_pallet/mod.rs @@ -236,7 +236,8 @@ //! by one character. FRAME errors are exactly a solution to maintain readability, whilst fixing //! the drawbacks mentioned. In short, we use an enum to represent different variants of our //! error. These variants are then mapped in an efficient way (using only `u8` indices) to -//! [`sp_runtime::DispatchError::Module`] Read more about this in [`frame::pallet_macros::error`]. +//! [`sp_runtime::DispatchError::Module`]. Read more about this in +//! [`frame::pallet_macros::error`]. //! //! - **Event**: Events are akin to the return type of dispatchables. They are mostly data blobs //! emitted by the runtime to let outside world know what is happening inside the pallet. Since