Handling skipped type params in subxt-codegen

[serban300]

Background context

The bridges team uses subxt-codegen in order to generate runtime types from metadata. We then use these types in order to submit bridge-related extrinsics to the chains.

Here is the code.

We substitute the types that are extrinsic arguments with the original types, instead of the ones generated by subxt-codegen. We encoutered an error recently with one of these types (sp_runtime::generic::header::Header). It has 2 type parameters (pub struct Header<Number: Copy + Into<U256> + TryFrom<U256>, Hash: HashT>), but recently a #[scale_info(skip_type_params(Hash))] has been added to the original structure. So subxt-codegen skips the Hash, leading to generated code that goes like:

				pub enum Call {
					#[codec(index = 0)]
					submit_finality_proof {
						finality_target: ::std::boxed::Box<
							runtime_types::sp_runtime::generic::header::Header<
								::core::primitive::u32,
							>,
						>,
						justification: ...
					},

But, since we substitute it with the original type which has 2 type params, we get compilation errors:

error[E0107]: struct takes 2 generic arguments but 1 generic argument was supplied
    --> relays/client-bridge-hub-westend/src/codegen_runtime.rs:1133:31
     |
1133 | ...                   ::sp_runtime::generic::Header<::core::primitive::u32>,
     |                                              ^^^^^^ ---------------------- supplied 1 generic argument
     |                                              |
     |                                              expected 2 generic arguments
     |
note: struct defined here, with 2 generic parameters: `Number`, `Hash`
    --> /home/bparity/.cargo/git/checkouts/polkadot-sdk-cff69157b985ed76/1cf7d3a/substrate/primitives/runtime/src/generic/header.rs:41:12
     |
41   | pub struct Header<Number: Copy + Into<U256> + TryFrom<U256>, Hash: HashT> {
     |            ^^^^^^ ------                                     ----
help: add missing generic argument
     |
1133 |                             ::sp_runtime::generic::Header<::core::primitive::u32, Hash>,

Request

I was wondering if it would make sense to keep the original definition of the structure (including the skipped type params) in such cases, for compatibility with type substitutions, even if the skipped typed params wouldn't actually be used if the type is not substituted.

I would be happy to help with the implementation if you don't have time.

[lexnv]

cc @tadeohepperle since I believe his work will soon handle this

[jsdw]

Ah interesting! I don't know the code in that area well enough to propose a solution, but just some thoughts and workaround ideas:

• Given that Hash doesn't impl TypeInfo (which is why it is skipped), we couldn't generate any type for it to use in place of that generic, and so if we did keep the generic type, what would go there in the generated code? Eg ::sp_runtime::generic::Header<::core::primitive::u32, ?>; what would go in the ?.
• In this example, the Hash type would need to implement HashT anyway, which the codegen type wouldn't (even if we did know how to generate such a type). So maybe it needs to be a custom type anyway rather than a generated one? (in some examples this wouldn't be true of course).

Which leads me to a couple of workaround ideas:

1. The type substitution allows you to also choose how to substitute generic params as part of it. In the case of Header, you need to add a generic type that makes sense for the "real" Header type. You can use syntax like this to do that:

TypeSubstitute::custom(
	"sp_runtime::generic::header::Header<T>",
	"::sp_runtime::generic::Header<T, ::some_type_that::MakesSense>",
),

2. If that doesn't work out, another approach is to define a custom type that wraps the type you want, like:

TypeSubstitute::custom(
	"sp_runtime::generic::header::Header<T>",
	"::path::to::MyCustomHeader<T>",
),

where you have something like:

struct MyCustomHeader<T>(::sp_runtime::generic::Header<T, ::some_type_that::MakesSense>);

This adds the flexibility of being able to derive whatever you need to etc on the custom type, and allows complete control over the generic alignment (some of which you can do using (1) now anyway).

And yup finally as Alex says, we have plans to move some of the core codegen stuff outside of subxt and give it an interface that will make it more generally useful (which then allows us to keep the subxt interfaces very specific to what makes sense for Subxt specifically) :)

[serban300]

@jsdw

Thank you for the answear and the suggestions ! You're right. I don't think the Hash type could be generated. But I don't think we can use custom types either. We don't want to hardcode the value of Hash.

Then I guess the only option would be to remove the #[scale_info(skip_type_params(Hash))]

[jsdw]

Then I guess the only option would be to remove the #[scale_info(skip_type_params(Hash))]

If it was removed, a custom TypeInfo impl would be needed to be enforced on whatever Hash type was provided there, which is a bit weird since it should be unnecessary. It would allow one thing though, which is capturing the type path/name for different concrete Hash types which were used (though may be tricky to make that be useful).

Ultimately, the issue is that we can't transfer logic through the metadata; only information about the name/shape of the types. So if you need custom logic (eg about the Hash logic), then the usual approach would be to implement custom types on your side to encapsulate the logic you want.

In subxt itself for instance, we have a Config trait which defines a bunch of associated types which have logic/behaviour that can be different across different chains. Then, for each chain, the user must select the correct implementation of this trait to ensure that the correct hashing logic or whatever is used for that chain. Perhaps a similar pattern could work for you?

[serban300]

If it was removed, a custom TypeInfo impl would be needed to be enforced on whatever Hash type was provided there, which is a bit weird since it should be unnecessary. It would allow one thing though, which is capturing the type path/name for different concrete Hash types which were used (though may be tricky to make that be useful).

Ultimately, the issue is that we can't transfer logic through the metadata; only information about the name/shape of the types. So if you need custom logic, then the usual approach is to implement custom types on your side to encapsulate the logic you want.

Makes sense

In subxt itself for instance, we have a Config trait which defines a bunch of associated types which can be different across different chains, and then for each chain the user must select the correct implementation of this trait to access the correct hash types and things for the chain. Perhaps a similar pattern could work for you?

We have some similar configs, but it would have been nice to get these types directly from the metadata. I'm thinking if we can use them.

Anyway, makes sense. Resolving this since it's not a subxt issue.

[serban300]

Thought about it some more.

Just as an example one of our use cases is the following. Relayers listen for finality proofs on some source chains and then sends these proofs + the finalized header to target chains using the submit_finality_proof extrinsic. At the target chain there is one submit_finality_proof extrinsic for each bridged chain. Let's say that we have the following bridges:

• Wococo -> Rococo
• Millau (an old test chain of ours) -> Rococo

So the relayers will send to Rococo through submit_finality_proof headers of type

• Header<u32, Blake2_256> (Blake2_256::Out = H256) from Wococo
• Header<u32, BlakeTwoAndKeccak256> (BlakeTwoAndKeccak256::Out = H512) from Millau

Before the #[scale_info(skip_type_params(Hash))] was added, by doing

TypeSubstitute::custom(
    "sp_runtime::generic::header::Header",
    "::sp_runtime::generic::Header",
),

The generated runtime call was accepting the original Wococo/Millau header, which was ideal for us:

				pub enum Call {
					#[codec(index = 0)]
					submit_finality_proof {
						finality_target: ::std::boxed::Box<
							::sp_runtime::generic::Header<
								::core::primitive::u32,
								::sp_runtime::traits::BlakeTwo256,
							>,
						>,

We could just take the header from the source chain and pass it to the target.

Right now for this scenario, subxt-codegen generates:

#[derive(:: codec :: Decode, :: codec :: Encode, Clone, Debug, PartialEq)]
pub struct Header<_0> {
    pub parent_hash: ::subxt::utils::H512,
    #[codec(compact)]
    pub number: _0,
    pub state_root: ::subxt::utils::H512,
    pub extrinsics_root: ::subxt::utils::H512,
    pub digest: ::sp_runtime::generic::Digest,
}

1. subxt-codegen uses this type everywhere:

pub enum Call {
    #[codec(index = 0)]
    submit_finality_proof {
        finality_target: ::std::boxed::Box<
            runtime_types::sp_runtime::generic::header::Header<
                ::core::primitive::u32,
            >,
        >,
        justification: ::bp_header_chain::justification::GrandpaJustification<
            runtime_types::sp_runtime::generic::header::Header<
                ::core::primitive::u32,
            >,
        >,
    },
    ...
}
...
#[derive(:: codec :: Decode, :: codec :: Encode, Clone, Debug, PartialEq)]
pub enum Call4 {
    #[codec(index = 0)]
    submit_finality_proof {
        finality_target: ::std::boxed::Box<
            runtime_types::sp_runtime::generic::header::Header<
                ::core::primitive::u32,
            >,
        >,
        justification: ::bp_header_chain::justification::GrandpaJustification<
            runtime_types::sp_runtime::generic::header::Header<
                ::core::primitive::u32,
            >,
        >,
    },
    ...
}

while the metadata specifies separate types:

name: submit_finality_proof
fields: [
  {
    name: finality_target
    type: 110
    typeName: Box<BridgedHeader<T, I>>
    docs: []
  }

name: submit_finality_proof
fields: [
  {
    name: finality_target
    type: 131
    typeName: Box<BridgedHeader<T, I>>
    docs: []
  }

I think this is a bug. I hope I didn't do something wrong while using subxt-codegen. But we would actually need 2 separate structures for Call and Call4 above. One that uses H256 and one that uses H512.

2. Not sure if it would be possible, but I think it would help us if instead of this, subxt-codegen could generate something like:

pub struct Header<_0, _1> {
    pub parent_hash: _1,
    #[codec(compact)]
    pub number: _0,
    pub state_root: _1,
    pub extrinsics_root: _1,
    pub digest: ::sp_runtime::generic::Digest,
}

Because then, having 2 generic params, we could actually create a custom adapter structure let's say ShadowHeader<Number, Hash>, substitute sp_runtime::generic::header::Header with our ShadowHeader when using subxt-codegen, and then we could have a method that transforms sp_runtime::generic::header::Header to ShadowHeader and call it when building the extrinsic. We would basically duplicate sp_runtime::generic::header::Header by creating a new ShadowHeader, but I think it would be acceptable.

And maybe it could also help fix the bug.

[jsdw]

Thanks for your reply!

I think this is a bug. I hope I didn't do something wrong while using subxt-codegen. But we would actually need 2 separate structures for Call and Call4 above. One that uses H256 and one that uses H512.

I suspect you're right; lemme check! The codegen stuff "deduplicates" concrete types that have generic params, but since one of the generic params is hidden in this Header type, I can imagine that instead there are two overlapping types that have the same path and different concrete values, and one of them ends up overwriting the other.

I'm not sure whether the solution would be to introduce another generic param or to just avoid the two different types from being squashed into one header type (the former would def be nicer, but I'm not sure offhand how easy it would be to add extra generics in this way). We'll look into it and see what we can do :)

[jsdw]

We basically iterate over all of the types and assume that two types with the same path are actually just the same type (and assume that therefore it must be that the generics that are different) here I think:

subxt/codegen/src/types/mod.rs

Line 101 in 6332196

innermost_module.types.insert(

We already have to dedupe duplicate pallets being used, which can lead to types with the same path, and we do that using this ensure_unique_path thing here:

subxt/codegen/src/api/mod.rs

Line 47 in 6332196

metadata.ensure_unique_type_paths();

I suspect the most reliable way to solve this bug is to extend that call to more reliably deduplicate types that have the same path but are different internally in any area that isn't explained by generic params.

Maybe an approach like:

1. Iterate all types in the registry, adding each to a hashmap with the full path+name as the key
2. If a type already exists at the given path, compare the type-to-be-added with the one in the hashmap
   • This would be almost just an equality check over the type info for the pair of types A and B, except that for any type ID that's mentioned that isn't already equal, we check if it's from one of the generic params, and if it's from the same generic param for A and B then it's still equal
3. If the type-to-be-added is not equal, then append an incrementing index and try to insert again, repeating the above and incrementing the index until we find either a match or no overlaps.

I think this should ensure that we never have dupe types based on paths in a more robust way than what we do currently.

This isn't as nice as adding a generic param to cater for the difference, and perhaps as a step 2, we can look at how feasible it is to add that; it would be nicer, but it would also be more complex, and could complement the above logic rather than replace it.

[tadeohepperle]

I implemented a better deduplication, in the way James described above, in scale-typegen in this PR which will hopefully resolve the bug, once we merge scale-typegen as a dependency into subxt (PR for that). I thought scale-typegen might be the appropriate place for this logic because we might need to deduplicate type paths in other projects as well.