Basic example pallet refactor #1707

sacha-l · 2023-09-25T22:53:18Z

The goal here is to simplify the basic example pallet to make it more readable and better highlight the features it demonstrates. Looking for initial feedback and comments (cc @kianenigma).

Here's a list of things I've done to that end:

removed the occurrence of setting on_initialize in the tests: we don't actually do anything with on_initialize, we could probably have another example pallet focuses on demonstrating it. I still left the on_initialize and on_finalize hooks in the pallet because the comments already there gives the reader the basic idea of how to declare those hooks and what they do
removed the off-chain worker hook: we already have an example pallet that showcases off-chain workers and it felt distracting to have it here as well
removed the MagicNumber constant: we have the default config example pallet to refer to already and we weren't demonstrating it's use here anyways
replaced the example of unsafe arithmetic in the commented section of accumulate_dummy and simplified it
added a helper function that reaps the storage of Dummy and DummyValueQuery to demonstrate the behaviour of ValueQuery and OptionQuery in tests

Other changes:

reorganized the layout of the pallet sections to follow the common patterns in most FRAME pallets
removed a lot of the comments that I think should really be in some reference doc crate, which I put in this hack MD until we have a place for that material
added docs to all storage items, types and functions
renamed Foo to DummyValueQuery to highlight the use of ValueQuery
renamed WatchDummy to WatchSetDummy to avoid confusion on what the SignedExtension is doing

Todo:

break up the tests to isolate the mutating values from checking the behaviour of the values returned after a value is removed from storage
add examples to the pallet docs using docify
Create example pallet demonstrating signed extensions
Create example pallet demonstrating do's and dont's for using hooks

kianenigma · 2023-09-26T08:56:13Z

could probably have another example pallet focuses on demonstrating it.

Yes, I very much like this. And in this example, really demonize on_initialize and similar in accordance with #198 (see "long term" section) and #165.

still left the on_initialize and on_finalize hooks in the pallet because the comments already there gives the reader the basic idea of how to declare those hooks and what they do

Fine with that, but please demonize them and highly encourage the reader to use alternatives such as:

Create a (possibly permissionless) dispatchable to do the same
use on_idle
wait for Tasks: general system for recognizing and executing service work #1343
This is a good demonstration of how you can avoid using HDH hooks: https://forum.polkadot.network/t/tellling-the-story-of-the-game-theory-behind-frame-pallets/2282/12?u=kianenigma

substrate/frame/examples/basic/Cargo.toml

substrate/frame/examples/basic/src/lib.rs

kianenigma · 2023-09-26T09:00:54Z

substrate/frame/examples/basic/src/lib.rs

@@ -82,6 +96,8 @@ pub use weights::*;

 /// A type alias for the balance type from this pallet's point of view.
 type BalanceOf<T> = <T as pallet_balances::Config>::Balance;
+
+/// A way to calculate transaction fees for our custom weight calculator.
 const MILLICENTS: u32 = 1_000_000_000;

 // A custom weight calculator tailored for the dispatch call `set_dummy()`. This actually examines


A pretty useful step is to also remove WeightForSetDummy and everything around it. We never implement this WeighData manually (see for yourself if you can find any), yet we are providing an example of it.

Instead, provide a reasonable and concise intro into what weights are, else keep them a constant for now.

Or make the pallet dev_mode.

In any case, this WeightForSetDummy is noise though in my opinion.

Yeah I agree - this was some stuff left from a long time ago. It could make for some good learning material about how weights work eventually - in particular implementing ClassifyDispatch and PaysFee. Though here it is noise. I'll remove it. This will also drastically simplify the pallet!

In the last commits I removed all custom weight stuff. Now I'm unsure whether to:

Provide proper benchmarked weights for set_dummy

OR make the pallet dev_mode

We already have a pallet that show cases dev_mode and I like how this example pallet provides the correct way for setting weights, plus using the macro for Inherited weight annotation for pallet calls which we show can be overriden, so I currently just set the weight for set_dummy to 0 to show this - though I'm weary about promoting this even as an example as it's not something any runtime dev should ever do.

Wdyt @kianenigma ?

substrate/frame/examples/basic/src/lib.rs

kianenigma · 2023-09-26T09:09:40Z

substrate/frame/examples/basic/src/lib.rs

 		}
 	}

-	// The call declaration. This states the entry points that we handle. The
+	/// Events are a simple means of reporting specific conditions and


weird line wrapping.

Does not seem resolved.

substrate/frame/examples/basic/src/lib.rs

kianenigma · 2023-09-26T09:14:42Z

substrate/frame/examples/basic/src/lib.rs

-	// Information about where this dispatch initiated from is provided as the first argument
-	// "origin". As such functions must always look like:
+	// The parenthesized value of the `#[pallet::weight(..)]` attribute can be any type that
+	// implements the following traits:


Yeah I would remove these. maybe, maybe a deep deep dive on weights can covert this. Otherwise for now I would just mention it can be any expression that returns either number, or (number, Pays) with a couple of examples.

Examples: https://github.com/Polkadot-Blockchain-Academy/pba-content/blob/cambridge-2022/syllabus/6-FRAME/6.2-Dispatchables/Dispatchables_slides.md#dispatchables-weight-examples-1

Unresolved.

I removed this in the last commits as per my last comment about removing custom weight stuff.

kianenigma · 2023-09-26T09:19:07Z

substrate/frame/examples/basic/src/lib.rs

-	// in system that do the matching for you and return a convenient result: `ensure_signed`,
-	// `ensure_root` and `ensure_none`.
+	// The weight for these calls relies on `WeightInfo`, which is auto-generated from the
+	// benchmark toolchain.
 	#[pallet::call(weight(<T as Config>::WeightInfo))]
 	impl<T: Config> Pallet<T> {


reviewed up until this point. Will do another pass once existing comments are addressed.

substrate/frame/examples/basic/src/lib.rs

wentelteefje · 2023-09-26T19:55:27Z

substrate/frame/examples/basic/src/lib.rs

+		///
+		/// In this call example, we use our own weight object `WeightForSetDummy` to determine the
+		/// weight of this call. This overrides the `WeightInfo` type used to derive weights for
+		/// other calls in this block.
 		#[pallet::call_index(1)]
 		#[pallet::weight(WeightForSetDummy::<T>(<BalanceOf<T>>::from(100u32)))]
 		pub fn set_dummy(
 			origin: OriginFor<T>,
 			#[pallet::compact] new_value: T::Balance,


I feel like the use of the #[pallet::compact] attribute macro here should be commented on, e.g. why would you want to use it when setting a balance, but not when increasing it (see function signature of accumulate_dummy)

sacha-l

Thanks for the preliminary review @kianenigma and @wentelteefje. Part of me looking for first impressions was to see whether it's reasonable to completely remove some of the things this pallet highlights (like SignedExtensions, Weights, and other distractions that ultimately should be either in their own ref docs or have their own pallet). And it seems we're aligned that this should be the direction I take. I've applied your suggestions and will continue improving and using docify in the actual pallet code to demonstrate behavior there.

substrate/frame/examples/basic/src/lib.rs

sacha-l · 2023-09-29T16:30:19Z

Since I'm essentially re-writing the basic example pallet, I've created a new pallet to isolate the examples around the basic use of StorageValue only. If we like this direction (of creating separate example pallets to demonstrate specific FRAME features), I'll go ahead and create others to demonstrate a signed extension and hooks in separate example pallets. These can become a useful place to easily grasp some of FRAME's core features in action.

Can you have another look @kianenigma and tell me what you think about this direction?

substrate/frame/examples/basic/src/lib.rs

kianenigma · 2023-10-02T16:18:13Z

substrate/frame/examples/basic/src/lib.rs

 	///
-	/// `frame_system::Config` should always be included.
+	/// Because all FRAME pallets depend on some core utility types from the System pallet,
+	/// [`frame_system::Config`] should always be included. This pallet example uses the `Balances`


Things like Config have some default doc that is generated by the pallet macro for them as well, FYI. If you provide your own docs, it will be overwritten.

substrate/frame/examples/basic/src/lib.rs

kianenigma

I have some general concerns about dummy-storage-value as a single example pallet (communicated elsewhere).

In any case, I suggest we keep this PR limited to just refactoring the basic example pallet.

This reverts commit 5037b55.

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

paritytech-cicd-pr · 2023-10-05T16:19:39Z

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

paritytech-cicd-pr · 2023-10-05T16:19:39Z

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

Sacha Lansky added 3 commits September 25, 2023 23:53

refactor basic example pallet

ba0ff2c

refactor tests

f0e9201

add docify and fmt

f80afe4

sacha-l added the T11-documentation This PR/Issue is related to documentation. label Sep 25, 2023

kianenigma requested a review from wentelteefje September 26, 2023 08:56

kianenigma reviewed Sep 26, 2023

View reviewed changes

substrate/frame/examples/basic/Cargo.toml Outdated Show resolved Hide resolved

kianenigma reviewed Sep 26, 2023

View reviewed changes

substrate/frame/examples/basic/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 26, 2023

View reviewed changes

substrate/frame/examples/basic/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 26, 2023

View reviewed changes

substrate/frame/examples/basic/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 26, 2023

View reviewed changes

substrate/frame/examples/basic/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 26, 2023

View reviewed changes

substrate/frame/examples/basic/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 26, 2023

View reviewed changes

substrate/frame/examples/basic/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 26, 2023

View reviewed changes

substrate/frame/examples/basic/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 26, 2023

View reviewed changes

substrate/frame/examples/basic/src/lib.rs Show resolved Hide resolved

kianenigma reviewed Sep 26, 2023

View reviewed changes

fmt cargo.toml

1ccb887

wentelteefje reviewed Sep 26, 2023

View reviewed changes

Sacha Lansky added 3 commits September 27, 2023 13:14

apply suggestions

960e20a

remove weight stuff

b53ef0e

update storage docs and add docify

9659e60

sacha-l commented Sep 27, 2023

View reviewed changes

substrate/frame/examples/basic/src/lib.rs Outdated Show resolved Hide resolved

substrate/frame/examples/basic/src/lib.rs Show resolved Hide resolved

Sacha Lansky added 2 commits September 29, 2023 18:07

add example dummy storage value pallet

5037b55

fix storage value docs

c7bb872

sacha-l requested review from kianenigma and wentelteefje September 29, 2023 16:30

kianenigma reviewed Oct 2, 2023

View reviewed changes

substrate/frame/examples/basic/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Oct 2, 2023

View reviewed changes

substrate/frame/examples/basic/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Oct 2, 2023

View reviewed changes

Sacha Lansky and others added 4 commits October 5, 2023 17:54

Revert "add example dummy storage value pallet"

f6e9b4b

This reverts commit 5037b55.

apply changes to basic example pallet

823a030

Update substrate/frame/examples/basic/src/lib.rs

976bedf

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

Update substrate/frame/examples/basic/src/lib.rs

c9ab45e

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

kianenigma closed this Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic example pallet refactor #1707

Basic example pallet refactor #1707

sacha-l commented Sep 25, 2023 •

edited

Loading

kianenigma commented Sep 26, 2023

kianenigma Sep 26, 2023 •

edited

Loading

kianenigma Sep 26, 2023

sacha-l Sep 27, 2023

sacha-l Oct 5, 2023

kianenigma Sep 26, 2023

kianenigma Oct 2, 2023

kianenigma Sep 26, 2023

kianenigma Sep 26, 2023

kianenigma Oct 2, 2023

sacha-l Oct 5, 2023

kianenigma Sep 26, 2023

wentelteefje Sep 26, 2023

sacha-l left a comment •

edited

Loading

sacha-l commented Sep 29, 2023

kianenigma Oct 2, 2023

kianenigma left a comment

paritytech-cicd-pr commented Oct 5, 2023

paritytech-cicd-pr commented Oct 5, 2023

Basic example pallet refactor #1707

Basic example pallet refactor #1707

Conversation

sacha-l commented Sep 25, 2023 • edited Loading

kianenigma commented Sep 26, 2023

kianenigma Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sacha-l left a comment • edited Loading

Choose a reason for hiding this comment

sacha-l commented Sep 29, 2023

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented Oct 5, 2023

paritytech-cicd-pr commented Oct 5, 2023

sacha-l commented Sep 25, 2023 •

edited

Loading

kianenigma Sep 26, 2023 •

edited

Loading

sacha-l left a comment •

edited

Loading