Investigate passing pallet index as const value

[bkchr]

When adding a pallet to construct_runtime!, an index for this pallet can be passed:

polkadot-sdk/polkadot/runtime/westend/src/lib.rs

Line 1418 in 814b938

Session: pallet_session::{Pallet, Call, Storage, Event, Config<T>} = 8,

In this case it would be 8. It would be nice to declare this 8 as a const value. Because I have seen requests like this more often recently. We will probably first need to fix parity-scale-codec to be able to accept a const value there. The main blocker will probably be the check for duplicate indices. However, we can probably work around this by creating some const function that is evaluating all the indices and panics in case of an error. We should check on the UX of this solution.

So the final solution would look the following:

const MY_PALLET_INDEX: usize = 10;

---
Session: pallet_session::{Pallet, Call, Storage, Event, Config<T>} = MY_PALLET_INDEX,

[ipapandinas]

hey @bkchr 🙋‍♂️ I'm eager to contribute to this issue, although I'm new to the Polkadot SDK codebase.

Regarding your mention of:

We will probably first need to fix parity-scale-codec to be able to accept a const value there.

Are you referring to adjusting this part of the code in the construct_runtime! macro:

polkadot-sdk/substrate/frame/support/procedural/src/construct_runtime/parse.rs

Lines 306 to 316 in 048a9c2

	// Parse for pallet index
	let index = if input.peek(Token![=]) {
	input.parse::<Token![=]>()?;
	let index = input.parse::<syn::LitInt>()?;
	let index = index.base10_parse::<u8>()?;
	Some(index)
	} else if !input.peek(Token![,]) && !input.is_empty() {
	return Err(input.error("Unexpected tokens, expected one of `=`, `,`"))
	} else {
	None
	};

If so, does this involve using syn::Expr to parse either a literal index or a path (which, in our case, would be a constant)?

[bkchr]

Are you referring to adjusting this part of the code in the construct_runtime! macro:

Yeah this is the place where we doing this for construct_runtime!, but you will find a similar code in parity-scale-codec for the codec(index = 1) parsing.

If so, does this involve using syn::Expr to parse either a literal index or a path (which, in our case, would be a constant)?

Yeah makes sense.

[ipapandinas]

Thank you for your advice, @bkchr. I have examined check_variant_attribute and variant_index in parity-scale-codec and propose the following updates to support both literal integers and constant expressions for pallet indexes.

check_variant_attribute

// in check_variant_attribute
NestedMeta::Meta(Meta::NameValue(meta_name_value))
	if meta_name_value.path.is_ident("index") => {
		match &meta_name_value.lit {
			Lit::Int(lit_int) => {
				lit_int.base10_parse::<u8>()
					.map(|_| ())
					.map_err(|_| syn::Error::new(lit_int.span(), "Index must be in 0..255"))
			},
			// Allow other general constant expressions but cannot validate them here
			_ => Ok(()),
		}
	},

variant_index

pub fn variant_index(v: &Variant, i: usize) -> TokenStream {
	// first look for an attribute
    let index_option = find_meta_item(v.attrs.iter(), |meta| {
        if let NestedMeta::Meta(Meta::NameValue(nv)) = meta {
            if nv.path.is_ident("index") {
                return match &nv.lit {
                    Lit::Int(lit_int) => {
                        lit_int.base10_parse::<u8>().ok().map(|byte| quote! { #byte })
                    },
                    // General constant expressions
                    _ => Some(quote! { #nv.lit }),
                };
            }
        }

        None
    });

    // then fallback to discriminant or just index
    index_option.unwrap_or_else(|| {
        v.discriminant
            .as_ref()
            .map(|(_, expr)| quote! { #expr })
            .unwrap_or_else(|| quote! { #i })
    })
}

Regarding the check for duplicate indices and your suggestion:

we can probably work around this by creating some const function that is evaluating all the indices and panics in case of an error

Here is a proposed solution:

1. Collect indices in a collected_indices vector before generating the encoding logic. This can be done after calling utils::variant_index in impl_encode and create_instance
2. Generate a specific const function to check duplicates with specific panic: let duplicate_idx_check_fn = generate_duplicate_idx_check_fn(collected_indices);
3. Include the duplicate const function in the final expanded macro output:

quote! {
	#duplicate_idx_check_fn
	impl #type_name {
		fn size_hint(&#self_) -> usize {
			#hinting
		}

		fn encode_to<__CodecOutputEdqy: #crate_path::Output + ?::core::marker::Sized>(
			&#self_,
			#dest: &mut __CodecOutputEdqy
		) {
			#encoding
		}
	}
}

What do you think of this approach for UX?

[bkchr]

Could you do a first POC and check how the panic would look like?

Best is probably to add some UI test like this one: https://github.com/paritytech/parity-scale-codec/blob/master/tests/max_encoded_len_ui/crate_str.stderr & https://github.com/paritytech/parity-scale-codec/blob/master/tests/max_encoded_len_ui/crate_str.rs

[ipapandinas]

I've created an initial POC addressing this issue in PR paritytech/parity-scale-codec#551 on the parity-scale-codec repository. Please take a look and let me know your thoughts @bkchr.

Additionally, if there are any other parts in the code we might need to update to thoroughly address this issue, do let me know.