Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

improve FRAME storage docs #13987

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Apr 24, 2023

I started by performing an exercise that I have suggested myself a few weeks ago: start writing a pallet, from scratch, with minimal copy-pasting. I have two goals:

  1. find out the DX shortcomings first-hand.
  2. build a new kitchensink-pallet that is a gallery of all features that the pallet-macro provides, see Ideas for a new set of example/template pallets polkadot-sdk#186.

This is related to 1. While coding a pallet, your IDE will give readily allow you to see the docs of two components:

  1. pallet::storage macro, which is coming from a stub in frame_support_procedural.
  2. the types defined in storage::types, eg StorageValue.

This PR moves all the relevant documentation of storage items into either of these two, while not repeating anything.

An open issue I have, which is also highlighted as a TODO in the code is:

I want to link to types like frame_support::storage::StorageValue in the frame_support_procedural macro stub rust-docs. Any way to do that? The code expanded by frame_support_procedural has access to frame-support, but I am not sure if the macro code itself can.

I would like to next suggest doing the same about weights and benchmarking, given its importance and being one of the biggest pain points for many.

@kianenigma kianenigma added the I6-documentation Documentation needs fixing, improving or augmenting. label Apr 24, 2023
/// }
/// ```
#[proc_macro_attribute]
pub fn constant(_: TokenStream, _: TokenStream) -> TokenStream {
pallet_macro_stub()
}

/// Declares an implementation block to be the dispatchable portion of a pallet. In other words,
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
/// Declares an implementation block to be the dispatchable portion of a pallet. In other words,
/// Declares an implementation block to be the callable portion of a pallet. In other words,

I see below that the trait Dispatchable shows up, but my initial intuition was to leave this word out of the docs. People understand callable. Dispatchable less so.

///
/// #[pallet::call]
/// impl<T: Config> Pallet<T> {
/// fn do_stuff(_origin: OriginFor<T>, arg: u32) {
Copy link
Member

Choose a reason for hiding this comment

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

Is weight no longer required? I think that is a good thing, but was surprised there wasn't dev mode stuff going on

Copy link
Member

Choose a reason for hiding this comment

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

It might be worth noting in the docs what call functions require? For example, it seems they all require an origin as the first argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in dev_mode it should no longer be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #14115 for exactly what you said about call.

Comment on lines +1211 to +1216
/// # use frame_support::pallet_prelude::*;
/// # #[pallet::config]
/// # pub trait Config: frame_system::Config {}
/// # #[pallet::pallet]
/// # pub struct Pallet<T>(_);
/// #[pallet::storage]
Copy link
Member

Choose a reason for hiding this comment

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

whats going on here with extra hashtags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will hide this line in the generated rust-docs. Try cargo doc to see examples.

///
/// If the keys are not trusted (e.g. can be set by a user), a cryptographic `hasher`
/// such as `blake2_128_concat` must be used for the key hashers. Otherwise, other values
/// in storage can be compromised.
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we remove this warning and information here ?

I think it is useful for user of StorageNMap to know how the storage is actually implemented and to be warns about its usage no ?

@stale
Copy link

stale bot commented Jul 23, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Jul 23, 2023
@stale stale bot closed this Aug 7, 2023
@kianenigma kianenigma reopened this Aug 14, 2023
@stale stale bot removed the A3-stale label Aug 14, 2023
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3373318

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3373317

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I6-documentation Documentation needs fixing, improving or augmenting. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

6 participants