use unit type in polkadot config

[tadeohepperle]

I changed the second generic parameter in the polkadot config to (), but I am a bit confused still about the default for substrate.

There are the two directories node and node-template in substate/bin.

node-template seems to use ():

/// The address format for describing accounts.
pub type Address = sp_runtime::MultiAddress<AccountId, ()>;

in substrate/bin/node-template/runtime/src/lib.rs

while node seems to use u32:

/// The type for looking up accounts. We don't expect more than 4 billion of them.
pub type AccountIndex = u32;

in substrate/bin/node/primitives/src/lib.rs and

/// The address format for describing accounts.
pub type Address = sp_runtime::MultiAddress<AccountId, AccountIndex>;

in substrate/bin/node/runtime/src/lib.rs

[lexnv]

I do wonder, we implement the Config directly to gain more flexibility for the PolkadotConfig?

[jsdw]

Yeah, WithExtrinsicParams only lets you change the extrinsic params bit; Tadeo needed to change another param so implementing Config "properly" is the way to go :)

[jsdw]

I changed the second generic parameter in the polkadot config to (), but I am a bit confused still about the default for substrate.

I might be wrong, but the question I think is, when you run substrate --dev, what config is being used. And for that it looks like the u32 is used :)

It's a good point though that people basing things off the node-template may have a slightly different config though.

I think what you've done here is all good for now; maybe we need to look through more things and provide more Config impls eventually (so long as we can test that they are keeping in sync with reality)

[jsdw]

Looks good to me!

[niklasad1]

pausing merge this....

we shouldn't target the node template in substrate it's different from the substrate --dev (it's called node)

https://github.com/paritytech/substrate/blob/master/bin/node/runtime/src/lib.rs#L1833 the AccountIndex is actually a u32, it might not matter for subxt anyway in most cases

I would prefer to have SubstrateNodeTemplateConfig and SubstrateNodeConfig or something SubstrateConfig is not obvious what it refer to but as most folks just run substrate --dev it's most reasonable to support by default

Yeah, PolkadotConfig is () but I'm not against removing the inheritance from SubstrateConfig

[tadeohepperle]

I would prefer to have SubstrateNodeTemplateConfig and SubstrateNodeConfig or something

If the two are different in the MultiAddress type we could have those two, I agree. I would like to know why those are different though, and having two types might confuse users a lot.
If we go that route, we could inherit PolkadotConfig from one of them again.

[jsdw]

My complete guess for why the two are different: the address type looks like:

pub enum MultiAddress<AccountId, AccountIndex> {
	/// It's an account ID (pubkey).
	Id(AccountId),
	/// It's an account index.
	Index(#[codec(compact)] AccountIndex),
	/// It's some arbitrary raw bytes.
	Raw(Vec<u8>),
	/// It's a 32 byte representation.
	Address32([u8; 32]),
	/// Its a 20 byte representation.
	Address20([u8; 20]),
}

And so Substrate uses u32 (essentially enabling Index as a way to specify an address), I guess because that's a more general case, whereas Polkadot has no use for it and so sets it to ().

Ultimately it won't affect anybody to use u32 instead of () except for making it possible for somebody to provide an address that isn't actually valid in Polkadot (and so this issue just exists to tighten that up).

I wouldn't be opposed to more Config types in general (though would like to be able to verify them better; maybe with the new extrinsic decoding we can just connect to some chain, download a block and check that we can decode it with the respective config). For now though I'm not against just leaving it as SubstrateConfig and maybe just adding configs for some of the common good parachains etc (with said test to sanity check) as needed (they may all just end up being basically the same; would need to actually look at the metadata/code for each to check!)

[jsdw]

So just to check,

This PR changes u32 into () for PolkadotConfig but doesn't touch the SubstrateConfig, so my view would be that we merge this (it's strictly more correct) and we can think about more/better named Configs as a separate issue (I wonder whether any of the common good parachain configs would differ or whether they all use () rather than u32 or what; would be a good little issue to investigate and see what the differences are!)

@niklasad1 what do you reckon?

[niklasad1]

So just to check,

This PR changes u32 into () for PolkadotConfig but doesn't touch the SubstrateConfig, so my view would be that we merge this (it's strictly more correct) and we can think about more/better named Configs as a separate issue (I wonder whether any of the common good parachain configs would differ or whether they all use () rather than u32 or what; would be a good little issue to investigate and see what the differences are!)

@niklasad1 what do you reckon?

Go ahead and merge it, missed that it didn't modify the SubstrateConfig so all good from myside