Skip to content

Commit

Permalink
Fix call enum's metadata regression (paritytech#3513)
Browse files Browse the repository at this point in the history
This fixes an issue introduced in
paritytech/substrate#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
  • Loading branch information
kianenigma authored Feb 29, 2024
1 parent 7f5d308 commit c0e52a9
Show file tree
Hide file tree
Showing 6 changed files with 157 additions and 103 deletions.
111 changes: 58 additions & 53 deletions docs/sdk/headers/header.html
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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();
Expand All @@ -82,58 +85,60 @@
</script>

<style>
body.sdk-docs {
nav.side-bar {
width: 300px;
}

nav.side-bar {
width: 300px;
}

.sidebar-table-of-contents {
margin-bottom: 1em;
padding: 0.5em;
}

.sidebar-table-of-contents a {
display: block;
margin: 0.2em 0;
}

.sidebar-table-of-contents .h2 {
font-weight: bold;
margin-left: 0;
}

.sidebar-table-of-contents .h3 {
margin-left: 1em;
}

.sidebar-table-of-contents .h4 {
margin-left: 2em;
}

.sidebar h2.location {
display: none;
}

.sidebar-elems {
display: none;
}

/* Center the 'Expand for More' button */
.expand-button {
display: inline-block; /* Use inline-block for sizing */
margin: 10px auto; /* Auto margins for horizontal centering */
padding: 5px 10px;
background-color: #007bff;
color: white;
text-align: center;
cursor: pointer;
border: none;
border-radius: 5px;
width: auto;
/* Centering the button within its parent container */
position: relative;
left: 50%;
transform: translateX(-50%);
}
.sidebar-table-of-contents {
margin-bottom: 1em;
padding: 0.5em;
}

.sidebar-table-of-contents a {
display: block;
margin: 0.2em 0;
}

.sidebar-table-of-contents .h2 {
font-weight: bold;
margin-left: 0;
}

.sidebar-table-of-contents .h3 {
margin-left: 1em;
}

.sidebar-table-of-contents .h4 {
margin-left: 2em;
}

.sidebar h2.location {
display: none;
}

.sidebar-elems {
display: none;
}

/* Center the 'Expand for More' button */
.expand-button {
display: inline-block;
/* Use inline-block for sizing */
margin: 10px auto;
/* Auto margins for horizontal centering */
padding: 5px 10px;
background-color: #007bff;
color: white;
text-align: center;
cursor: pointer;
border: none;
border-radius: 5px;
width: auto;
/* Centering the button within its parent container */
position: relative;
left: 50%;
transform: translateX(-50%);
}
}
</style>
25 changes: 13 additions & 12 deletions docs/sdk/headers/theme.css
Original file line number Diff line number Diff line change
@@ -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;
}
}
18 changes: 18 additions & 0 deletions prdoc/pr_3513.prdoc
Original file line number Diff line number Diff line change
@@ -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
21 changes: 16 additions & 5 deletions substrate/frame/support/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
21 changes: 3 additions & 18 deletions substrate/frame/support/procedural/src/pallet/expand/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
use crate::{
pallet::{
expand::warnings::{weight_constant_warning, weight_witness_warning},
parse::call::{CallVariantDef, CallWeightDef},
parse::call::CallWeightDef,
Def,
},
COUNTER,
Expand Down Expand Up @@ -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::<Vec<_>>();
let fn_doc = methods.iter().map(|method| &method.docs).collect::<Vec<_>>();

let args_name = methods
.iter()
Expand Down Expand Up @@ -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 {
#(
Expand Down
64 changes: 49 additions & 15 deletions substrate/frame/support/test/tests/pallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ pub mod pallet {
where
T::AccountId: From<SomeType1> + From<SomeType3> + 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(
Expand All @@ -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(
Expand Down Expand Up @@ -270,7 +270,7 @@ pub mod pallet {
#[pallet::error]
#[derive(PartialEq, Eq)]
pub enum Error<T> {
/// doc comment put into metadata
/// error doc comment put in metadata
InsufficientProposersBalance,
NonExistentStorageValue,
Code(u8),
Expand All @@ -287,9 +287,8 @@ pub mod pallet {
where
T::AccountId: SomeAssociation1 + From<SomeType1>,
{
/// doc comment put in metadata
/// event doc comment put in metadata
Proposed(<T as frame_system::Config>::AccountId),
/// doc
Spending(BalanceOf<T>),
Something(u32),
SomethingElse(<T::AccountId as SomeAssociation1>::_1),
Expand Down Expand Up @@ -750,8 +749,7 @@ pub type UncheckedExtrinsic =
sp_runtime::testing::TestXt<RuntimeCall, frame_system::CheckNonZeroSender<Runtime>>;

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,
Expand All @@ -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(|| {
Expand Down Expand Up @@ -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::<pallet::Call<Runtime>>().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::<pallet::Event<Runtime>>().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::<pallet::Error<Runtime>>().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];

Expand Down

0 comments on commit c0e52a9

Please sign in to comment.