From c0e52a9ed61451152c4e096c58f24fb4e329bd64 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Thu, 29 Feb 2024 19:08:08 +0000 Subject: [PATCH] Fix call enum's metadata regression (#3513) This fixes an issue introduced in https://github.com/paritytech/substrate/pull/14101, in which I removed the `Call` enum's documentation and replaced it with a link to the `Pallet` struct, but this also removed any docs related to call from the metadata. I tried to add a regression test for this, but it seems to me that this is not possible, given that using `type-info` we only assert in type-ids for `Call`, `Event` and `Error`. I removed some doc comments from a test setup in `frame-support-test` to demonstrate the issue there. @jsdw do you have any comments on this? I also fixed a small issue in the custom html/css of `polkadot-sdk-doc` crate, making sure it does not affect the rust-doc page of all other crates. - [x] Investigate a regression test - [x] prdoc --- docs/sdk/headers/header.html | 111 +++++++++--------- docs/sdk/headers/theme.css | 25 ++-- prdoc/pr_3513.prdoc | 18 +++ substrate/frame/support/Cargo.toml | 21 +++- .../procedural/src/pallet/expand/call.rs | 21 +--- substrate/frame/support/test/tests/pallet.rs | 64 +++++++--- 6 files changed, 157 insertions(+), 103 deletions(-) create mode 100644 prdoc/pr_3513.prdoc diff --git a/docs/sdk/headers/header.html b/docs/sdk/headers/header.html index 68dc5d5509e6..e28458c4ccc7 100644 --- a/docs/sdk/headers/header.html +++ b/docs/sdk/headers/header.html @@ -54,7 +54,7 @@ sidebarElems.style.display = 'none'; // Add click event listener to the button - expandButton.addEventListener('click', function() { + expandButton.addEventListener('click', function () { // Toggle the display of the '.sidebar-elems' if (sidebarElems.style.display === 'none') { sidebarElems.style.display = 'block'; @@ -72,6 +72,9 @@ if (!crate_name.textContent.startsWith("polkadot_sdk_docs")) { console.log("skipping -- not `polkadot_sdk_docs`"); return; + } else { + // insert class 'sdk-docs' to the body, so it enables the custom css rules. + document.body.classList.add("sdk-docs"); } createToC(); @@ -82,58 +85,60 @@ diff --git a/docs/sdk/headers/theme.css b/docs/sdk/headers/theme.css index bb9254ec4a82..a488e15c36b7 100644 --- a/docs/sdk/headers/theme.css +++ b/docs/sdk/headers/theme.css @@ -1,16 +1,17 @@ :root { - --polkadot-pink: #E6007A ; - --polkadot-green: #56F39A ; - --polkadot-lime: #D3FF33 ; - --polkadot-cyan: #00B2FF ; - --polkadot-purple: #552BBF ; - } - -body > nav.sidebar > div.sidebar-crate > a > img { - /* logo width, given that the sidebar width is 200px; */ - width: 190px; + --polkadot-pink: #E6007A; + --polkadot-green: #56F39A; + --polkadot-lime: #D3FF33; + --polkadot-cyan: #00B2FF; + --polkadot-purple: #552BBF; } -body nav.sidebar { - flex: 0 0 250px; +body.sdk-docs { + nav.sidebar>div.sidebar-crate>a>img { + width: 190px; + } + + nav.sidebar { + flex: 0 0 250px; + } } diff --git a/prdoc/pr_3513.prdoc b/prdoc/pr_3513.prdoc new file mode 100644 index 000000000000..e1f2afe280a5 --- /dev/null +++ b/prdoc/pr_3513.prdoc @@ -0,0 +1,18 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Fix call enum's metadata regression + +doc: + - audience: Runtime Dev + - audience: Runtime User + description: | + This PR fixes an issue where in the metadata of all FRAME-based chains, the documentation for + all extrinsic/call/transactions has been replaced by a single line saying "see Pallet::name". + + Many wallets display the metadata of transactions to the user before signing, and this bug + might have affected this process. + +crates: + - name: frame + - name: frame-support diff --git a/substrate/frame/support/Cargo.toml b/substrate/frame/support/Cargo.toml index 72e2d0bfa569..c19f478c803e 100644 --- a/substrate/frame/support/Cargo.toml +++ b/substrate/frame/support/Cargo.toml @@ -18,13 +18,24 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] array-bytes = { version = "6.1", default-features = false } serde = { features = ["alloc", "derive"], workspace = true } -codec = { package = "parity-scale-codec", version = "3.6.1", default-features = false, features = ["derive", "max-encoded-len"] } -scale-info = { version = "2.10.0", default-features = false, features = ["derive"] } -frame-metadata = { version = "16.0.0", default-features = false, features = ["current"] } -sp-api = { path = "../../primitives/api", default-features = false, features = ["frame-metadata"] } +codec = { package = "parity-scale-codec", version = "3.6.1", default-features = false, features = [ + "derive", + "max-encoded-len", +] } +scale-info = { version = "2.10.0", default-features = false, features = [ + "derive", +] } +frame-metadata = { version = "16.0.0", default-features = false, features = [ + "current", +] } +sp-api = { path = "../../primitives/api", default-features = false, features = [ + "frame-metadata", +] } sp-std = { path = "../../primitives/std", default-features = false } sp-io = { path = "../../primitives/io", default-features = false } -sp-runtime = { path = "../../primitives/runtime", default-features = false, features = ["serde"] } +sp-runtime = { path = "../../primitives/runtime", default-features = false, features = [ + "serde", +] } sp-tracing = { path = "../../primitives/tracing", default-features = false } sp-core = { path = "../../primitives/core", default-features = false } sp-arithmetic = { path = "../../primitives/arithmetic", default-features = false } diff --git a/substrate/frame/support/procedural/src/pallet/expand/call.rs b/substrate/frame/support/procedural/src/pallet/expand/call.rs index f43faba1ee0c..f395872c8a80 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/call.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/call.rs @@ -18,7 +18,7 @@ use crate::{ pallet::{ expand::warnings::{weight_constant_warning, weight_witness_warning}, - parse::call::{CallVariantDef, CallWeightDef}, + parse::call::CallWeightDef, Def, }, COUNTER, @@ -112,22 +112,7 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream { } debug_assert_eq!(fn_weight.len(), methods.len()); - let map_fn_docs = if !def.dev_mode { - // Emit the [`Pallet::method`] documentation only for non-dev modes. - |method: &CallVariantDef| { - let reference = format!("See [`Pallet::{}`].", method.name); - quote!(#reference) - } - } else { - // For the dev-mode do not provide a documenation link as it will break the - // `cargo doc` if the pallet is private inside a test. - |method: &CallVariantDef| { - let reference = format!("See `Pallet::{}`.", method.name); - quote!(#reference) - } - }; - - let fn_doc = methods.iter().map(map_fn_docs).collect::>(); + let fn_doc = methods.iter().map(|method| &method.docs).collect::>(); let args_name = methods .iter() @@ -309,7 +294,7 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream { ), #( #cfg_attrs - #[doc = #fn_doc] + #( #[doc = #fn_doc] )* #[codec(index = #call_index)] #fn_name { #( diff --git a/substrate/frame/support/test/tests/pallet.rs b/substrate/frame/support/test/tests/pallet.rs index 7bee4b7c8206..607f54ccc131 100644 --- a/substrate/frame/support/test/tests/pallet.rs +++ b/substrate/frame/support/test/tests/pallet.rs @@ -209,7 +209,7 @@ pub mod pallet { where T::AccountId: From + From + SomeAssociation1, { - /// Doc comment put in metadata + /// call foo doc comment put in metadata #[pallet::call_index(0)] #[pallet::weight(Weight::from_parts(*foo as u64, 0))] pub fn foo( @@ -225,7 +225,7 @@ pub mod pallet { Ok(().into()) } - /// Doc comment put in metadata + /// call foo_storage_layer doc comment put in metadata #[pallet::call_index(1)] #[pallet::weight({1})] pub fn foo_storage_layer( @@ -270,7 +270,7 @@ pub mod pallet { #[pallet::error] #[derive(PartialEq, Eq)] pub enum Error { - /// doc comment put into metadata + /// error doc comment put in metadata InsufficientProposersBalance, NonExistentStorageValue, Code(u8), @@ -287,9 +287,8 @@ pub mod pallet { where T::AccountId: SomeAssociation1 + From, { - /// doc comment put in metadata + /// event doc comment put in metadata Proposed(::AccountId), - /// doc Spending(BalanceOf), Something(u32), SomethingElse(::_1), @@ -750,8 +749,7 @@ pub type UncheckedExtrinsic = sp_runtime::testing::TestXt>; frame_support::construct_runtime!( - pub struct Runtime - { + pub struct Runtime { // Exclude part `Storage` in order not to check its metadata in tests. System: frame_system exclude_parts { Pallet, Storage }, Example: pallet, @@ -772,6 +770,14 @@ fn _ensure_call_is_correctly_excluded_and_included(call: RuntimeCall) { } } +fn maybe_docs(doc: Vec<&'static str>) -> Vec<&'static str> { + if cfg!(feature = "no-metadata-docs") { + vec![] + } else { + doc + } +} + #[test] fn transactional_works() { TestExternalities::default().execute_with(|| { @@ -1362,19 +1368,47 @@ fn migrate_from_pallet_version_to_storage_version() { }); } +#[test] +fn pallet_item_docs_in_metadata() { + // call + let call_variants = match meta_type::>().type_info().type_def { + scale_info::TypeDef::Variant(variants) => variants.variants, + _ => unreachable!(), + }; + + assert_eq!(call_variants[0].docs, maybe_docs(vec!["call foo doc comment put in metadata"])); + assert_eq!( + call_variants[1].docs, + maybe_docs(vec!["call foo_storage_layer doc comment put in metadata"]) + ); + assert!(call_variants[2].docs.is_empty()); + + // event + let event_variants = match meta_type::>().type_info().type_def { + scale_info::TypeDef::Variant(variants) => variants.variants, + _ => unreachable!(), + }; + + assert_eq!(event_variants[0].docs, maybe_docs(vec!["event doc comment put in metadata"])); + assert!(event_variants[1].docs.is_empty()); + + // error + let error_variants = match meta_type::>().type_info().type_def { + scale_info::TypeDef::Variant(variants) => variants.variants, + _ => unreachable!(), + }; + + assert_eq!(error_variants[0].docs, maybe_docs(vec!["error doc comment put in metadata"])); + assert!(error_variants[1].docs.is_empty()); + + // storage is already covered in the main `fn metadata` test. +} + #[test] fn metadata() { use codec::Decode; use frame_metadata::{v15::*, *}; - fn maybe_docs(doc: Vec<&'static str>) -> Vec<&'static str> { - if cfg!(feature = "no-metadata-docs") { - vec![] - } else { - doc - } - } - let readme = "Support code for the runtime.\n\nLicense: Apache-2.0\n"; let expected_pallet_doc = vec![" Pallet documentation", readme, readme];