-
Notifications
You must be signed in to change notification settings - Fork 103
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
base: main
Are you sure you want to change the base?
use RuntimeGenesisConfig in genesis config presets #451
Conversation
@dharjeezy clippy is not happy. |
Review required! Latest push from author must always be reviewed |
@dharjeezy please add a CHANGELOG entry too |
Co-authored-by: Adrian Catangiu <[email protected]>
CHANGELOG.md
Outdated
- Added Polkadot <> Kusama bridge to support asset transfers between Asset Hubs ([polkadot-fellows/runtimes#108](https://github.com/polkadot-fellows/runtimes/pull/108)) | ||
|
||
### Fixed |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
@dharjeezy PR needs resolving merge conflicts |
…sis-config # Conflicts: # CHANGELOG.md
@dharjeezy the CI is still failing |
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(), |
There was a problem hiding this comment.
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.
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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michalkucharczyk wdyt here also?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
@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, now:
and should be:
|
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michalkucharczyk wdyt here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 thepallet-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
There was a problem hiding this comment.
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.
…sis-config # Conflicts: # CHANGELOG.md # system-parachains/encointer/src/genesis_config_presets.rs
A new macro, |
…sis-config # Conflicts: # CHANGELOG.md
This PR is now ready to be merged @bkontur |
Makes use of
RuntimeGenesisConfig
to construct genesis config values for all runtimes.closes #431