Vision: Improve Usage of Constants in Pallets

[shawntabrizi]

At the moment, all configurations in the runtime use trait Get<T> in order to pass those configurations from the runtime definition to the pallet.

However, many of these configurations are assumed to be constant, while others are not. At the Configuration API level, we provide no distinction for these differences, and thus are prone to errors caused by runtime misconfigurations.

To improve this, I propose the following changes:

• Add a ConstGet<T> trait, with documentation that any use of this trait must be implemented by a true constant value.
• Generate a tiny bit of macro code which will wrap a const in a trait into a ConstGet<T> so it can be used within the runtime for things like BoundedVec
• Change BoundedVec to use ConstGet<T> rather than just Get<T>
• Go through pallets and migrate all those which can easily convert Get<T> into const where it makes sense.
• Those that do not migrate easily, migrate to ConstGet<T> where it makes sense.
• Update paramter_types! macro to generate ConstGet<T> on const, but not things like static and storage

[shawntabrizi]

The hard part of this exercise is to properly identify which Get<T> functions should be turned into const, ConstGet<T> or stay Get<T>.

Unfortunately we cannot migrate everything which SHOULD be a const into that since const generic features are not fully stabilized: paritytech/substrate#9865

[Doordashcon]

Hi @shawntabrizi can i take on this issue

[KiChjang]

@Doordashcon Yes, please go ahead! If you need any help, please ping me or Shawn in the Substrate Technical channel on Element/Discord.

[Doordashcon]

@KiChjang sure thing

[Doordashcon]

@KiChjang want to submit task by task?

[shawntabrizi]

I think the first two tasks can be done in 1 PR. Make sure your PR targets paritytech/substrate, not your fork

[Doordashcon]

is this the general idea for the second task?

#[impl_const]
trait ConstantId {
  const ID: i32;
}

the attribute-like macro #[impl_const] will implement ConstGet<T> for associated constants in ConstantId

impl ConstGet<i32> for i32 {
  fn get() -> i32
}

[shawntabrizi]

@Doordashcon yes, but ideally within the trait itself?

trait ConstantId {
  #[impl_const_get]
  const ID: i32;
}

Is that possible?

[Doordashcon]

I don't think the macro would know where it is called, so it cannot know about the ConstantId trait, which is a needed element for the expansion.

Another approach would be to keep the attribute on the trait definition, but require a helper annotation on const

#[apply(derive_Gets!)]
trait ConstantId {
    #[Get]
    const ID: i32;
}

And have the outer macro skip the associated items with no #[Get] applied to them.

I would use a crate called macro_use_attribute with the alias apply which allows the use of declarative macros as proc_macro attributes or derives.

[KiChjang]

Generate a tiny bit of macro code which will wrap a const in a trait into a ConstGet so it can be used within the runtime for things like BoundedVec

This syntax:

trait ConstantId {
  #[impl_const_get]
  const ID: i32;
}

is actually doable in FRAME, assuming that ConstantId means the pallet's Config trait. Even if ConstantId means something else, we can make it so that any associated constant defined inside of a trait under #[frame_support::pallet] would parse for an attribute above itself.

[danielhenrymantilla]

The syntax

trait ConstantId {
    #[impl_const_get]
    const ID: i32;
}

would —within a sane implementation— be unable to generate an impl … block since that's not a valid assoc item for trait ConstantId definition:

//! Expanded code

trait ConstantId {
    const ID: i32;

    impl … // Uh oh
}

While there exists a contrived workaround to allow for such an impl … declaration nested inside the const ID: i32, I highly recommend that the trait just take an external annotation:

//! Expansion: incorrect Rust

#[preprocessor_name] // <- actual proc-macro attribute   <-------------------------------------------------+
trait ConstantId {                                                                                      // |
    #[const_get] // <- fake proc-macro attr / inert attr that shall just act as a syntactical marker for --+
    const ID: i32;
}

It's way easier to implement (and the result is more readable). In fact, for a non-generic trait, with only associated constants, that shall be picked or ignored by the "preprocessor", then such a preprocessor can be implemented with a macro_rules! macro, that can then be, if so desired, #[apply]ed as an attribute.

• Demo

[KiChjang]

@danielhenrymantilla That is my point, we already have #[frame_support::pallet] as a preprocessor for all pallet definitions.

See the directory under frame/support/procedural/src/pallet for more information.

[Lohann]

We needed this feature in our pallet, the workaround we found is using traits, but is not optimal:

trait ConstGet<T> {
    const VALUE: T;
}

struct Two;
impl ConstGet<usize> for Two {
    const VALUE: usize = 2;
}

struct Four;
impl ConstGet<usize> for Four {
    const VALUE: usize = 4;
}

struct FixedArray<const N: usize>([u32; N]);
impl <const N: usize> FixedArray<N> {
    fn new() -> Self {
        Self([0;N])
    }
}

type TwoByteArray = FixedArray::<{Two::VALUE}>;
type FourByteArray = FixedArray::<{Four::VALUE}>;

fn main() {
    let a = TwoByteArray::new();
    let b = FourByteArray::new();
    println!("a: {:?}", a.0);
    println!("b: {:?}", b.0);
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=daee4712ad219a12546465b8ddf774bf

Then the Config definition is something like this:

/// Configuration trait.
#[pallet::config]
pub trait Config: frame_system::Config {
    ...
    // Constant array size
    type ArraySize: ConstGet<usize>;
}

[benluelo]

Is there any more action on this? I noticed that paritytech/substrate#10287 was closed a year ago; I'd be interested in picking this up as it would be nice to have as a precursor for #290 (and #250).

[Doordashcon]

Hey @benluelo, you can pick this up :)

[kianenigma]

I have a little bit of a grudge against this line of work as there is nothing at the language level that enforced ConstGet to actually be const, as done in paritytech/parity-common#722. This is assuming everyone uses parameter_types! and as soon as you implement ConstGet manually you can still have it be backed by storage or whatever.

Given this, I would just let go of this goal, instead of trying to give a pallet the false sense that something is constant (via type Foo: ConstGet<u32>) while it is not.

Instead, I would focus on #244, which is about detection of Get<_> changes, which kinda trumps the need to enforce any const-ness.