Instantiate All Assets Pallets

[joepetrowski]

One step toward #1856

This should be a no-op in the runtime except for the migration of storage keys. It's done as a preparation for an upgrade that will add another instance to the chains.

Open to another name than "Trust Backed Assets", although they do tend to have privileged roles corresponding to accessible accounts.

[shawntabrizi]

AFAIK you do not need to add Instance1 for the default/first instance. You can just add instances when you want to add new ones. The default one can stay "instanceless" which will still make it unique among instantiated pallets.

[joepetrowski]

AFAIK you do not need to add Instance1 for the default/first instance. You can just add instances when you want to add new ones. The default one can stay "instanceless" which will still make it unique among instantiated pallets.

Interesting, OK I'll try a demo with that. We still need the underlying changes to some of the other traits to deal with multiple instances of Assets.

[joepetrowski]

AFAIK you do not need to add Instance1 for the default/first instance. You can just add instances when you want to add new ones. The default one can stay "instanceless" which will still make it unique among instantiated pallets.

Interesting, OK I'll try a demo with that. We still need the underlying changes to some of the other traits to deal with multiple instances of Assets.

I tried doing this but ran into a lot of errors when generics in other structs asked for a specific instance of the Assets pallet. Is there any downside to just leaving an explicit Instance?

[bkchr]

Mainly renamings and changing of the prefix. Looks good, but the weight needs to be corrected for each migration.

[gilescope]

@joepetrowski if you want you could define pub type TrustBackedAssetsInstance = (); and construct_runtime macro can stay as Assets: pallet_assets::{Pallet, Call, Storage, Event<T>} = 50,). - that seems to build ok on my machine. That would keep it using the default instance.

[joepetrowski]

OK, is there any advantage to not using the instance? IMO Instance1 is more expressive than ().

[bkontur]

OK, is there any advantage to not using the instance? IMO Instance1 is more expressive than ().

I agree and I am curious also,
for some bridge pallets I needed to add Instance2 and without Instance1 it looked strange: why just Instance2 and where is Instance1? so, I ended up with explicit Instance1/Instance2

[bkontur]

... That would keep it using the default instance.

@gilescope @joepetrowski
TrustBackedAssetsInstance vs ForeignCreatorAssetsInstance, in theory, can we even say that one of them is default? I guess, they are two "equivalent" instances and none is default-like, right?

[joepetrowski]

@gilescope @joepetrowski TrustBackedAssetsInstance vs ForeignCreatorAssetsInstance, in theory, can we even say that one of them is default? I guess, they are two "equivalent" instances and none is default-like, right?

I agree in general, but I think Default vs. Instance1 vs. Instance2 etc. just has to do with the macro expansion in the bin. The pallets' storage and API get exposed by their name in construct_runtime, so Assets, ForeignAssets.

For example, in Polkadot, Council and Tech Comm are Council and TechnicalCollective, not pallet_collectives::Instance{1, 2}.

[gilescope]

I'm thinking that () would equate to PalletInstance 0 in XCM and Instance1 would equate to PalletInstance 1 so () would not change the xcm multilocation of the assets.

[joepetrowski]

I thought PalletInstance() is the index of the pallet in the runtime, e.g. https://github.com/paritytech/cumulus/blob/9e211b8/parachains/runtimes/assets/westmint/tests/tests.rs#L61

Likewise, the docs say, "An instanced, indexed pallet that forms a constituent part of the context." I think the context is the runtime, not "all instances of pallet_assets".

@KiChjang can confirm?

I.e., PalletInstance is 50, not 1:

Assets: pallet_assets::<Instance1>::{Pallet, Call, Storage, Event<T>} = 50,

[gilescope]

Ah that's a good point. Ok happy that the assets will not change their addressable location.

[KiChjang]

@joepetrowski is correct.

[joepetrowski]

bot merge