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

Improve FRAME storage docs #1714

Merged
merged 26 commits into from
Nov 1, 2023
Merged

Conversation

wentelteefje
Copy link
Contributor

@wentelteefje wentelteefje commented Sep 26, 2023

This is a port (and hopefully a small improvement) of @kianenigma's PR from the old Substrate repo: paritytech/substrate#13987. Following #1689 I moved the documentation of all macros relevant to this PR from frame_support_procedural to pallet_macros while including a hint for RA users.

Question: Again with respect to #1689: Is there a good reason why we should not enhance paths with links to our current rustdocs? For example, instead of

/// **Rust-Analyzer users**: See the documentation of the Rust item in
/// `frame_support::pallet_macros::storage`.

we could write

/// **Rust-Analyzer users**: See the documentation of the Rust item in
/// [`frame_support::pallet_macros::storage`](https://paritytech.github.io/polkadot-sdk/master/frame_support/pallet_macros/attr.storage.html).

This results in a clickable link like this:
image
I don't really expect the links to become outdated any time soon, but I think this would be a great UX improvement over just having paths.

TODOs:

  • Add documentation for constant_name macro
  • Add proper documentation for different QueryKinds, i.e. OptionQuery, ValueQuery, ResultQuery. One example for each. Custom OnEmpty should be moved to QueryKinds trait doc page.
  • Rework type_value docs

@wentelteefje wentelteefje added T1-FRAME This PR/Issue is related to core FRAME, the framework. T11-documentation This PR/Issue is related to documentation. labels Sep 26, 2023
@wentelteefje wentelteefje requested review from a team and kianenigma September 26, 2023 12:04
@wentelteefje wentelteefje marked this pull request as draft September 26, 2023 21:15
@wentelteefje wentelteefje mentioned this pull request Sep 26, 2023
4 tasks
/// #[pallet::storage_prefix = "OtherFoo"]
/// #[pallet::unbounded]
/// pub type Foo<T> = StorageValue<_, u32, ValueQuery>;
/// }
Copy link
Contributor

Choose a reason for hiding this comment

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

another idea is for you to add a single test/snippet here to each storage type where you re-create the storage key to demonstrate the point. Something like:

fn main() {
	let expected_key = twox(pallet_name) ++ twox(Foo)
	let key = Foo::hashed_key();
	assert_eq!(expected., key);
}

Sadly would require you to build a runtime, but thanks to all the recent work it is only a few lines of code now :)

/// #[frame_support::pallet]
/// mod pallet {
/// use frame_support::pallet_prelude::*;
///
Copy link
Contributor

Choose a reason for hiding this comment

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

newlines can probably go away for readability?

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

@kianenigma kianenigma Sep 27, 2023

Choose a reason for hiding this comment

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

we can faily easily add something like this:

fn main() {
	let call = pallet::Call::do_stuff { .. }
	let origin = ..
	call.dispatch(origin);
}

///
/// The following macros can be used in conjunction with the storage macro:
///
/// * [`macro@getter`]: Creates a custom getter function.
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 these don't fit great into the story so far. Moving them toward the end might improve, but no sure. Do you think they are well explained in this position, in the overall narrative?

/// ### Hashers
///
/// For all storage types, except `StorageValue`, a set of hashers needs to be specified. The choice
/// of hashers is crucial, especially in production chains. The purpose of
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one that I would probably tackle in a ref-doc, as it is too detailed. Good for now I would say.

/// Lastly, it's recommended for hashers with "concat" to have reversible hashes. Refer to the
/// implementors section of [`hash::ReversibleStorageHasher`](frame_support::hash::ReversibleStorageHasher).
///
/// ### Prefixes
Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, so maybe you don't need to cover the key generation details of each storage type in each type, and tackle them all here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have given an example for StorageValue and StorageMap. However, I'm still thinking about whether I should add them additionally for each type in their respective pages...

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Looks mostly good, but I have some improvement suggestions.

Next, I suggest you to tackle pallet::Config. It is probably the last "important" one to tackle among the foundations of pallets.

I think that would also be a good alignment exercise for us, so I can see how you would tackle one from scratch and give you feedback. Because I had written the foundations of this myself, it was hard for me to measure.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3991823

Copy link
Contributor

@sam0x17 sam0x17 left a comment

Choose a reason for hiding this comment

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

great use of docify!

@paritytech-review-bot paritytech-review-bot bot requested review from a team November 1, 2023 13:24
@kianenigma kianenigma marked this pull request as ready for review November 1, 2023 14:09
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

LGTM, it is not perfect but HUGE step in the right direction, rather merge soon and iterate later.

It is part of paritytech/polkadot-sdk-docs#31

Copy link
Contributor

@CrackTheCode016 CrackTheCode016 left a comment

Choose a reason for hiding this comment

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

This looks really good for a first iteration. We can still of course provide a bit more clarity on the dependnecies that storage types may use, and how they relate, etc (as well as improve things like the kitchensink bit), but this is a huge improvement!

@wentelteefje
Copy link
Contributor Author

LGTM, it is not perfect but HUGE step in the right direction, rather merge soon and iterate later.

Yes, please go ahead. I still have quite a few ideas, but for these I'll open separate PRs later on. Curious to see how this develops :)

@kianenigma
Copy link
Contributor

@wentelteefje feel free to sign up to continuing with this or other macros in paritytech/polkadot-sdk-docs#31

@kianenigma kianenigma merged commit b6965af into master Nov 1, 2023
14 of 18 checks passed
@kianenigma kianenigma deleted the wentelteefje-improve-storage-docs branch November 1, 2023 15:28
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
This is a port (and hopefully a small improvement) of @kianenigma's PR
from the old Substrate repo:
paritytech/substrate#13987. Following paritytech#1689 I
moved the documentation of all macros relevant to this PR from
`frame_support_procedural` to `pallet_macros` while including a hint for
RA users.

Question: Again with respect to paritytech#1689: Is there a good reason why we
should *not* enhance paths with links to our current rustdocs? For
example, instead of
```rust
/// **Rust-Analyzer users**: See the documentation of the Rust item in
/// `frame_support::pallet_macros::storage`.
```
we could write
```rust
/// **Rust-Analyzer users**: See the documentation of the Rust item in
/// [`frame_support::pallet_macros::storage`](https://paritytech.github.io/polkadot-sdk/master/frame_support/pallet_macros/attr.storage.html).
```
This results in a clickable link like this:
<img width="674" alt="image"
src="https://github.com/paritytech/polkadot-sdk/assets/10713977/c129e622-3942-4eeb-8acf-93ee4efdc99d">
I don't really expect the links to become outdated any time soon, but I
think this would be a great UX improvement over just having paths.

TODOs:
- [ ] Add documentation for `constant_name` macro
- [x] Add proper documentation for different `QueryKinds`, i.e.
`OptionQuery`, `ValueQuery`, `ResultQuery`. One example for each. Custom
`OnEmpty` should be moved to `QueryKinds` trait doc page.
- [ ] Rework `type_value` docs

---------

Co-authored-by: kianenigma <kian@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework. T11-documentation This PR/Issue is related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants