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

Pallet dispatchable+storage doc module. #13341

Merged
merged 20 commits into from
Mar 15, 2023
Merged

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Feb 8, 2023

hopefully closes #12398, but needs more work.
step toward #13299

Description

The goal of this PR is to make docs for dispatchables and storages on pallets more accessible and discoverable when browsing the generated docs. The PR accomplishes this by adding two auto-generated modules to all pallets: dispatchables and storage_types. These modules are only produced when building docs and otherwise do not exist or interfere with pallets. The storage_types module is auto-populated with documented structs representing each storage type from the parent pallet. The dispatchables module is auto-populated with uncallable auto-generated functions matching the signature of each Call variant. Right now a link to the corresponding Call enum variant is also included, though this may be removed. A note is included specifying that these cannot be called and are doc-only.

Modules added to pallet module:

image

Detail view of storage_types module:

image

Detail view of of dispatchables module:

Screenshot 2023-03-14 at 5 11 58 PM

Detail view of a particular dispatchable:

Screenshot 2023-03-14 at 5 12 45 PM

Also hides docs for Instance2-Instance16 on the pallet module and adds a doc comment to Instance1 explaining this:

image

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 8, 2023
@kianenigma kianenigma changed the title doc-only pallet Pallet dispatchable+storage doc module. Feb 8, 2023
@sam0x17 sam0x17 self-assigned this Feb 14, 2023
@sam0x17
Copy link
Contributor

sam0x17 commented Mar 8, 2023

Figuring out how we want to handle generics in fn signatures in pallets aka situations like this:

		#[pallet::call_index(2)]
		#[pallet::weight(T::WeightInfo::force_transfer())]
		pub fn force_transfer(
			origin: OriginFor<T>,
			source: AccountIdLookupOf<T>,
			dest: AccountIdLookupOf<T>,
			#[pallet::compact] value: T::Balance,
		) -> DispatchResultWithPostInfo {

are causing this error:

error[E0412]: cannot find type `T` in this scope
   --> frame/balances/src/lib.rs:372:28
    |
369 |         pub fn force_transfer(
    |                              - help: you might be missing a type parameter: `<T>`
...
372 |             dest: AccountIdLookupOf<T>,
    |                                     ^ not found in this scope

As far as I can tell that is the only outstanding thing not working

@kianenigma
Copy link
Contributor Author

Figuring out how we want to handle generics in fn signatures in pallets aka situations like this:

		#[pallet::call_index(2)]
		#[pallet::weight(T::WeightInfo::force_transfer())]
		pub fn force_transfer(
			origin: OriginFor<T>,
			source: AccountIdLookupOf<T>,
			dest: AccountIdLookupOf<T>,
			#[pallet::compact] value: T::Balance,
		) -> DispatchResultWithPostInfo {

are causing this error:

error[E0412]: cannot find type `T` in this scope
   --> frame/balances/src/lib.rs:372:28
    |
369 |         pub fn force_transfer(
    |                              - help: you might be missing a type parameter: `<T>`
...
372 |             dest: AccountIdLookupOf<T>,
    |                                     ^ not found in this scope

As far as I can tell that is the only outstanding thing not working

Maybe put a <T: Config> on every function? or better, wrap them all in

/// Some doc saying that this struct does not exist in reality
pub struct Dispatchables<T: Config>(Phantom(T));
impl<T: Config> Dispatchables {
  // This is now 100% identical to `Pallet` struct and should work. 
}

and then we export struct Dispatchables, which is good enough.

@sam0x17 sam0x17 added D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. C1-low PR touches the given topic and has a low impact on builders. labels Mar 9, 2023
@sam0x17 sam0x17 marked this pull request as ready for review March 9, 2023 14:57
@bkchr
Copy link
Member

bkchr commented Mar 9, 2023

This clearly requires some screenshots on how that looks in the docs. I'm not 100% sure that adding fake items to the docs is such a great idea.

@sam0x17
Copy link
Contributor

sam0x17 commented Mar 9, 2023

Screenshot 2023-03-09 at 12 17 51 AM

These when clicked take you to the relevant docs. I think the best dev UX would be to have the docs here verbatim but alas that doesn't quite work in rustdoc with re-exports. I actually ran into a literal compiler bug when I tried to do anything other than re-exporting for this one, btw.
Screenshot 2023-03-09 at 12 25 06 AM

This one works better because they are not re-exports:
Screenshot 2023-03-09 at 4 02 14 PM

@bkchr
Copy link
Member

bkchr commented Mar 9, 2023

Did you tried using pub type StorageItem = super::StorageItem instead of the re-export.

But with the calls people will then not know how to use them. From the docs it looks like there is some module, which doesn't really exists 🤷

@sam0x17
Copy link
Contributor

sam0x17 commented Mar 9, 2023

Did you tried using pub type StorageItem = super::StorageItem instead of the re-export.

But with the calls people will then not know how to use them. From the docs it looks like there is some module, which doesn't really exists 🤷

Yeah in that case I think the current solution is the best compromise because at least it points them to the right place for the types.

As far as accessibility, I will add a comment saying these types and functions are available on the pallet itself or something to that effect

@kianenigma
Copy link
Contributor Author

Suggestion: For each dispatchable, link to the actually function in struct Pallet and the corresponding enum Call variant as well, emphasizing that "the real thing is there, this is just a fake" list.

For storage items, I think this is great and already a massive improvement.

I will try and help a bit with the auto-generated documents, making sure they are not confusing, especially for the dispatch stuff.

Copy link
Contributor Author

@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.

I think it is a good step forward, but perhaps we can chose the doc strings a bit better.

Can't approve as I opened the PR, but has my blessing.

@sam0x17
Copy link
Contributor

sam0x17 commented Mar 10, 2023

Hmm I wonder if we could instead just add the docs to the Call enum variants as an alternative to the dispatchables module? thoughts?

@KiChjang
Copy link
Contributor

As with everything doc-related, it's hard to give comments if we don't know how it looks like on the UI, especially when it is about grouping similar items together, which is what the entire underlying issue is about.

@sam0x17
Copy link
Contributor

sam0x17 commented Mar 10, 2023

As with everything doc-related, it's hard to give comments if we don't know how it looks like on the UI, especially when it is about grouping similar items together, which is what the entire underlying issue is about.

i.e. what if we emit doc strings on these variants instead of the dispatchables module shown above:
Screenshot 2023-03-10 at 2 12 41 AM

@KiChjang
Copy link
Contributor

I still haven't seen how the docs on the enum variants look like, but I'm assuming it's similar to https://docs.rs/syn/latest/syn/enum.Type.html, in which case, it's clearly an improvement. We might want to write some doc comments on the Call enum itself so that people know how to click into it and look for docs on the dispatchables.

@sam0x17
Copy link
Contributor

sam0x17 commented Mar 10, 2023

I still haven't seen how the docs on the enum variants look like, but I'm assuming it's similar to https://docs.rs/syn/latest/syn/enum.Type.html, in which case, it's clearly an improvement. We might want to write some doc comments on the Call enum itself so that people know how to click into it and look for docs on the dispatchables.

Yeah I haven't seen it yet either as it will require moving parts of the macro around to a whole different part of the pallet expansion. And yeah, I think this would be better dev UX since the Call enum is the actual thing that gets generated

@sam0x17
Copy link
Contributor

sam0x17 commented Mar 10, 2023

lol so we've come full circle. I scrolled down the page on the Call enum only to realize the variants are already documented:
Screenshot 2023-03-10 at 8 19 14 AM

Do we maybe just want to surface this better with a link to Call on the main docs page for each pallet? Looks like this already perfectly documents the dispatchables for any given pallet..

@sam0x17
Copy link
Contributor

sam0x17 commented Mar 10, 2023

I guess the intent though of what @kianenigma was doing with the dispatchables was to render them in the docs as actual functions instead of enum variants. Given that, I think it might be better to leave it the way he set it up, but I'm going to add doc links to the actual variants like he suggested on each one

@sam0x17
Copy link
Contributor

sam0x17 commented Mar 11, 2023

OK so yeah latest commit adds this preface to the docs for the auto-generated dispatchables stub functions:

Screenshot 2023-03-11 at 2 23 27 AM

Notably, the NOTE actually links to the proper enum variant corresponding with the function.

@sam0x17
Copy link
Contributor

sam0x17 commented Mar 14, 2023

Have also added a revamp of the very ugly Instance1, Instance2, Instance3... Instance16 stuff in the pallet module based on @ggwpez's feedback:

image

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member

ggwpez commented Mar 14, 2023

Using structs for the storage types works. We dont actually need the concrete storage type. If we really want we could still link to the frame-support::storage::... type but for high-level this looks good to me.
Before:
image
After:
image

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
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.

Looks much better now, thanks!

@sam0x17
Copy link
Contributor

sam0x17 commented Mar 14, 2023

@KiChjang this should be good for a final look now, screenshots at the top are updated

@sam0x17 sam0x17 requested a review from bkchr March 15, 2023 01:15
frame/support/procedural/src/pallet/expand/doc_only.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/pallet/expand/doc_only.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/pallet/expand/doc_only.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/pallet/expand/doc_only.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/pallet/expand/doc_only.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/pallet/expand/doc_only.rs Outdated Show resolved Hide resolved
sam0x17 and others added 7 commits March 15, 2023 09:14
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Bastian Köcher <git@kchr.de>
@sam0x17
Copy link
Contributor

sam0x17 commented Mar 15, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@sam0x17
Copy link
Contributor

sam0x17 commented Mar 15, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit d3fd543 into master Mar 15, 2023
@paritytech-processbot paritytech-processbot bot deleted the doc-macro branch March 15, 2023 16:36
ukint-vs pushed a commit to gear-tech/substrate that referenced this pull request Apr 10, 2023
* doc-only pallet

* cargo fmt

* generics fix for dispatchables

* use a module instead

* add doc comment warning that the dispatchable functions are generated

* clean up

* fix typo

* hide Instance4-Instance16 from `pallet` module docs

* revamp Instance1-16 comment

* Document storage types

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* fmt

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* remove unused variables

* crate::Call => Call

Co-authored-by: Bastian Köcher <git@kchr.de>

* fix indentation

Co-authored-by: Bastian Köcher <git@kchr.de>

* remove unneeded block

Co-authored-by: Bastian Köcher <git@kchr.de>

* don't need a Vec

Co-authored-by: Bastian Köcher <git@kchr.de>

* add "doc only" to coment

Co-authored-by: Bastian Köcher <git@kchr.de>

* crate::Call => Call

Co-authored-by: Bastian Köcher <git@kchr.de>

* cargo fmt

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Sam Johnson <sam@durosoft.com>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: parity-processbot <>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* doc-only pallet

* cargo fmt

* generics fix for dispatchables

* use a module instead

* add doc comment warning that the dispatchable functions are generated

* clean up

* fix typo

* hide Instance4-Instance16 from `pallet` module docs

* revamp Instance1-16 comment

* Document storage types

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* fmt

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* remove unused variables

* crate::Call => Call

Co-authored-by: Bastian Köcher <git@kchr.de>

* fix indentation

Co-authored-by: Bastian Köcher <git@kchr.de>

* remove unneeded block

Co-authored-by: Bastian Köcher <git@kchr.de>

* don't need a Vec

Co-authored-by: Bastian Köcher <git@kchr.de>

* add "doc only" to coment

Co-authored-by: Bastian Köcher <git@kchr.de>

* crate::Call => Call

Co-authored-by: Bastian Köcher <git@kchr.de>

* cargo fmt

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Sam Johnson <sam@durosoft.com>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc-only module to group all storage items of a pallet
5 participants