Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate: Rename SubstrateKitchensinkConfig to something more generic #586

Closed
haerdib opened this issue Jun 14, 2023 · 10 comments · Fixed by #604
Closed

Investigate: Rename SubstrateKitchensinkConfig to something more generic #586

haerdib opened this issue Jun 14, 2023 · 10 comments · Fixed by #604
Assignees

Comments

@haerdib
Copy link
Contributor

haerdib commented Jun 14, 2023

From encointer/encointer-node#328 (comment):

But it seems to be generic enough such that it works for us too. So it could be something like DefaultAssetRuntimeConfig.

I agree with that line of thought. SubstratKitchensinkConfig is correct. But maybe a little too confining.

@echevrier
Copy link
Contributor

According to Substrate, the node example runtime is NOTHING production-like, and should not be used, nor inspired from ;) That's why they've renamed the node-runtime to node-kitchensink-runtime.
Changing the name of our configuration is then deceptively reassuring, but I can understand, that for live nodes, referring to a Kitchensink is not so nice.

We have another config that is compatible with a polkadot node: PolkadotConfig . If we rename SubstratKitchensinkConfig without reference to Substrate, we should also rename PolkadotConfig.

A little too confining? I'm not sure where to put the emphasis. For the moment the differences between the configs are the methods of payment or the nodes that use them. If we don't want to make any reference to node-kitchensink-runtime, I'd like to remain as neutral as possible. So maybe DefaultAssetRuntimeConfig instead of SubstratKitchensinkConfig and DefaultRuntimeConfig instead of PolkadotConfig?

Other possibilities

  • DefaultSubstrateAssetRuntimeConfig and DefaultPolkadotRuntimeConfig?
  • SubstrateAssetRuntimeConfig and PoladotRuntimeConfig?

@haerdib , @clangenb What do you think?

@clangenb
Copy link
Collaborator

clangenb commented Jul 1, 2023

Yes, I can understand the rationale why they renamed it into the kitchensink runtime. However, if the config works for polkadot and kusama, I think it shouldn't have the kitchensink name in it. I can agree with your second set of suggestions, but would like to add some more variants:

  • AssetRuntimeConfig, PolkadotConfig

I don't have a strong tendency towards your or mine suggestions.

@haerdib
Copy link
Contributor Author

haerdib commented Jul 3, 2023

Maybe I'm a little behind here, but for me, the difference between Polkadot and Substrate is not clear in regard to the config. Both, Substrate and Polkadot nodes may have both types of Payments, and both, substrate-node-template (not kitchsink-runtime, as @echevrier noted should not be used as template), and polkadot nodes by default use plain tip.

Maybe I'm wrong here? But if not, I'd refrain from using Polkadot or Substrate in the naming, but instead focus on Asset and Plain or Default ?

Suggestions:

  • AssetConfig, PlainConfig
  • DefaultAssetConfig, DefaultConfig
  • AssetRuntimeConfig, RuntimeConfig

@echevrier
Copy link
Contributor

So let's go with DefaultAssetConfig and DefaultPlainConfig. Emphasis on Default and on Asset/Plain.

@clangenb
Copy link
Collaborator

clangenb commented Jul 4, 2023

I think asset and plain are a bit too mysterious. As a slight adjustment, I would suggest:

DefaultAssetPaymentConfig, DefaultNativePaymentConfig although I am not sure about the latter. :)

@haerdib
Copy link
Contributor Author

haerdib commented Jul 4, 2023

I agree. Not sure about Native as well though.

How about: AssetPaymentConfig, DefaultPaymentConfig?

Because the plain tip is certainly default, while it is probably debatable if asset could be labelled as Default.

@clangenb
Copy link
Collaborator

clangenb commented Jul 4, 2023

Yes, I think this is OK, I can agree with your suggestions. My primary concern before was that PlainTip has been coined by subxt, and is not a term known to the general substrate/polkadot dev.

@echevrier
Copy link
Contributor

AssetPaymentConfig, DefaultPaymentConfig sounds like payment configs. I would prefer AssetRuntimeConfig and DefaultRuntimeConfig if we can't use Plain.

@clangenb
Copy link
Collaborator

clangenb commented Jul 4, 2023

Sounds good to me!

@haerdib
Copy link
Contributor Author

haerdib commented Jul 5, 2023

Yes, that makes sense 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants