Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

implement GenesisConfig for pallet-xcm #4125

Merged
merged 2 commits into from
Oct 28, 2021

Conversation

zjb0807
Copy link
Contributor

@zjb0807 zjb0807 commented Oct 22, 2021

Support to modify the genesis config in TestExternalities.

@KiChjang
Copy link
Contributor

KiChjang commented Oct 25, 2021

Can you elaborate the problem that you're encountering and why this implementation is necessary to solve it?

@zjb0807
Copy link
Contributor Author

zjb0807 commented Oct 26, 2021

Can you elaborate the problem that you're encountering and why this implementation is necessary to solve it?

Support to modify the genesis config in TestExternalities.

pallet_xcm::GenesisConfig { safe_xcm_version: Some(2) }
		.assimilate_storage::<Test>(&mut t)
		.unwrap();

The same as
https://github.com/paritytech/substrate/blob/732f0371c2d4da7b0ec326c681e0c1d360fc1bd8/frame/system/src/lib.rs#L740-L758

@KiChjang
Copy link
Contributor

@ascjones I see that you have authored the code at https://github.com/paritytech/substrate/blob/732f0371c2d4da7b0ec326c681e0c1d360fc1bd8/frame/system/src/lib.rs#L740-L758, would you kindly explain why it was necessary, and whether this PR needs to solve the same problem?

@ascjones
Copy link
Contributor

@KiChjang it was simply to ease migration from the old-style FRAME macros to @thiolliere's FRAME-V2 macros, and is not stricly necessary. See the guide here: https://docs.substrate.io/rustdocs/latest/frame_support/attr.pallet.html#upgrade-guidelines. I think it was just to avoid having to update various tests etc.

@KiChjang
Copy link
Contributor

@zjb0807 So if your intention was simply to patch the code so it works with FRAMEv1 macros, then it isn't necessary. Do you have any other problems that you're trying to solve with this PR?

@zjb0807
Copy link
Contributor Author

zjb0807 commented Oct 27, 2021

@zjb0807 So if your intention was simply to patch the code so it works with FRAMEv1 macros, then it isn't necessary. Do you have any other problems that you're trying to solve with this PR?

I have said it many times to modify the genesis config in TestExternalities.

In order to add the following code in mock.rs

pallet_xcm::GenesisConfig { safe_xcm_version: Some(2) }
		.assimilate_storage::<Test>(&mut t)
		.unwrap();

@KiChjang
Copy link
Contributor

KiChjang commented Oct 27, 2021

@zjb0807 I also asked you to elaborate the problem , but you did not.

I have said it many times to modify the genesis config in TestExternalities.
In order to add the following code in mock.rs

What you just said here simply tells me what your PR does, it does not tell me why you need to do it the way that you have.

In order to really find out what the issue is, I've tried to call assimilate_storage for XCM pallet's GenesisConfig, and it resulted in an error claiming that type annotations are required for the GenesisBuild type parameter. This sounds like it could easily be fixed by UFCS.

@zjb0807
Copy link
Contributor Author

zjb0807 commented Oct 27, 2021

What you just said here simply tells me what your PR does, it does not tell me why you need to do it the way that you have.

There is GenesisConfig in the xcm pallet to configure the default version, and developers can customize or use the default value in TestExternalities. But now it returns an error.
So I implement GenesisConfig like frame-system to solve compilation problems.

xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
@@ -324,6 +324,10 @@ pub(crate) fn new_test_ext_with_balances(
.assimilate_storage(&mut t)
.unwrap();

pallet_xcm::GenesisConfig { safe_xcm_version: Some(2) }
.assimilate_storage::<Test>(&mut t)
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is good practice to build the storages for all the pallets as would do a real genesis build. so this change could be kept.
WDYT @KiChjang ?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is fine for me, although this would not work as-is -- it would need your UFCS syntax in order to work.

@gui1117 gui1117 added A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Oct 28, 2021
@KiChjang
Copy link
Contributor

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 16e67d2 into paritytech:master Oct 28, 2021
emostov pushed a commit that referenced this pull request Nov 1, 2021
* implement GenesisConfig for pallet-xcm

* Apply review suggestions
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants