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

use RuntimeGenesisConfig in genesis config presets #451

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

dharjeezy
Copy link
Contributor

@dharjeezy dharjeezy commented Sep 2, 2024

Makes use of RuntimeGenesisConfig to construct genesis config values for all runtimes.

closes #431

@bkchr
Copy link
Contributor

bkchr commented Sep 3, 2024

@dharjeezy clippy is not happy.

Copy link

github-actions bot commented Sep 3, 2024

Review required! Latest push from author must always be reviewed

@github-actions github-actions bot requested a review from bkchr September 3, 2024 21:34
@acatangiu
Copy link
Contributor

@dharjeezy please add a CHANGELOG entry too

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Adrian Catangiu <[email protected]>
CHANGELOG.md Outdated
Comment on lines 363 to 364
- Added Polkadot <> Kusama bridge to support asset transfers between Asset Hubs ([polkadot-fellows/runtimes#108](https://github.com/polkadot-fellows/runtimes/pull/108))

### Fixed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the empty line was accidentally removed 😆 you need to bring it back (but empty, no whitespaces)

aura_ext: Default::default(),
polkadot_xcm: PolkadotXcmConfig {
_config: Default::default(),
safe_xcm_version: Some(SAFE_XCM_VERSION),
},
// no need to pass anything to aura, in fact it will panic if we do. Session will take care
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment needs to go to line 58, at this position it does not make so much sense.

aura_ext: Default::default(),
polkadot_xcm: PolkadotXcmConfig {
_config: Default::default(),
safe_xcm_version: Some(SAFE_XCM_VERSION),
},
// no need to pass anything to aura, in fact it will panic if we do. Session will take care
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@acatangiu
Copy link
Contributor

@dharjeezy PR needs resolving merge conflicts

@bkchr
Copy link
Contributor

bkchr commented Sep 10, 2024

@dharjeezy the CI is still failing

Comment on lines +61 to +71
aura: Default::default(),
aura_ext: Default::default(),
polkadot_xcm: PolkadotXcmConfig {
_config: Default::default(),
safe_xcm_version: Some(SAFE_XCM_VERSION),
},
assets: Default::default(),
foreign_assets: Default::default(),
parachain_system: Default::default(),
vesting: Default::default(),
pool_assets: Default::default(),
Copy link
Contributor

@bkontur bkontur Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dharjeezy Is it intentional to name all the genesis fields here? Could we just use ..Default::default(), for them? Every time we add a new pallet to the runtime, we would also need to add xyz = Default::default() here.

Suggested change
aura: Default::default(),
aura_ext: Default::default(),
polkadot_xcm: PolkadotXcmConfig {
_config: Default::default(),
safe_xcm_version: Some(SAFE_XCM_VERSION),
},
assets: Default::default(),
foreign_assets: Default::default(),
parachain_system: Default::default(),
vesting: Default::default(),
pool_assets: Default::default(),
polkadot_xcm: PolkadotXcmConfig {
_config: Default::default(),
safe_xcm_version: Some(SAFE_XCM_VERSION),
},
..Default::default(),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalkucharczyk wdyt here also?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..Default::default() shall be used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also adding link to related discussion: paritytech/polkadot-sdk#5327 (comment)

@bkchr
Copy link
Contributor

bkchr commented Sep 11, 2024

@dharjeezy the "Workspace features" CI test is still failing.

@dharjeezy
Copy link
Contributor Author

@dharjeezy the "Workspace features" CI test is still failing.

I see it is the coretime-polkadot-runtime benchmarks that is failing, when i tried running the coretime-polkadot-runtime benchmarks locally, they all seem to pass, i am still trying to figure out what the issue is.

@bkontur
Copy link
Contributor

bkontur commented Sep 11, 2024

@dharjeezy the "Workspace features" CI test is still failing.

I see it is the coretime-polkadot-runtime benchmarks that is failing, when i tried running the coretime-polkadot-runtime benchmarks locally, they all seem to pass, i am still trying to figure out what the issue is.

hmm, I have a feeling that this might be related to the: paritytech/polkadot-sdk#5083 (comment), also we are working on fix: paritytech/polkadot-sdk#5655,

but to confirm my assumption, I will prepare a draft PR above this PR with frame-omni-bencher built from that branch with fix

@bkontur
Copy link
Contributor

bkontur commented Sep 11, 2024

@dharjeezy the "Workspace features" CI test is still failing.

I see it is the coretime-polkadot-runtime benchmarks that is failing, when i tried running the coretime-polkadot-runtime benchmarks locally, they all seem to pass, i am still trying to figure out what the issue is.

hmm, I have a feeling that this might be related to the: paritytech/polkadot-sdk#5083 (comment), also we are working on fix: paritytech/polkadot-sdk#5655,

but to confirm my assumption, I will prepare a draft PR above this PR with frame-omni-bencher built from that branch with fix

@dharjeezy ok, no, ignore my previous comment,
the problem is that you accidentally removed SAFE_XCM_VERSION here: https://github.com/polkadot-fellows/runtimes/pull/451/files#diff-f5accd9c71b73f488f5376cf30987209be62e8c81ab3ca4e25c47e1fae65be9dL62-L64

now:

polkadot_xcm: PolkadotXcmConfig { _config: Default::default(), safe_xcm_version: None },

and should be:

		polkadot_xcm: PolkadotXcmConfig {
			_config: Default::default(),
			safe_xcm_version: Some(SAFE_XCM_VERSION),
		},

@dharjeezy
Copy link
Contributor Author

polkadot_xcm: PolkadotXcmConfig {
_config: Default::default(),
safe_xcm_version: Some(SAFE_XCM_VERSION),
},

Ah...ok. Thank you

@@ -66,3 +67,20 @@ where

/// The default XCM version to set in genesis config.
pub const SAFE_XCM_VERSION: u32 = xcm::prelude::XCM_VERSION;

pub fn remove_phantom_fields(value: &mut Value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dharjeezy I think that we don't need to remove anything here, e.g. these phantoms... so I would revert remove_phantom_fields stuff. It is not enough just phantom, e.g. some of the pallets use _config or config or _phantom. The pallet's GeneisConfig should not serialize those, e.g. paritytech/polkadot-sdk@6226e84

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalkucharczyk wdyt here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[#serde(skip)] shall be used for phantom fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means i have to open a PR in polkadot-sdk to include the [#serde(skip)] annotation for these pallets because they don't actually have them example is the pallet-bridge-messages pallet? @michalkucharczyk @bkontur

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, PR shall be opened. In that case, IMO we could keep this function until clean-up PR is merged (some annotations would be nice).

As alternative (and probably better solution) you could experiment with removing the fields that were not customized from the final JSON (see this discussion).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means i have to open a PR in polkadot-sdk to include the [#serde(skip)] annotation for these pallets because they don't actually have them example is the pallet-bridge-messages pallet? @michalkucharczyk @bkontur

@dharjeezy actually, this pallet was fixed already in the master: https://github.com/paritytech/polkadot-sdk/blob/master/bridges/modules/messages/src/lib.rs#L539-L540, but if you have there any others, please feel free to open PR to polkadot-sdk

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just saw that it is now added, we will have to wait for a new release.

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

Successfully merging this pull request may close these issues.

Consider using RuntimeGenesisConfig instead of directly constructing json for the preset genesis patch
7 participants