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

export xcm_pallet config #4116

Merged

Conversation

GopherJ
Copy link
Contributor

@GopherJ GopherJ commented Oct 21, 2021

Signed-off-by: Cheng JIANG [email protected]

close: #4115

cc: @KiChjang

Not sure if I should touch kusama's genesis config, but it can be useful to be able to test against kusama-local

Signed-off-by: Cheng JIANG <[email protected]>
Signed-off-by: Cheng JIANG <[email protected]>
@KiChjang KiChjang added A0-please_review Pull request needs code review. 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 21, 2021
Copy link
Contributor

@KiChjang KiChjang left a comment

Choose a reason for hiding this comment

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

Exporting the Config part from XCM pallet is fine, I just want to ensure that we're not trying to change any sort of config on Kusama (though it's rather pointless since Kusama's already launched and GenesisConfig won't ever affect it again...)

node/service/src/chain_spec.rs Outdated Show resolved Hide resolved
node/service/src/chain_spec.rs Outdated Show resolved Hide resolved
node/service/src/chain_spec.rs Outdated Show resolved Hide resolved
node/service/src/chain_spec.rs Outdated Show resolved Hide resolved
Signed-off-by: Cheng JIANG <[email protected]>
@GopherJ
Copy link
Contributor Author

GopherJ commented Oct 21, 2021

Exporting the Config part from XCM pallet is fine, I just want to ensure that we're not trying to change any sort of config on Kusama (though it's rather pointless since Kusama's already launched and GenesisConfig won't ever affect it again...)

yes that's why I changed it, because now I have to test against kusama-local and westend-local is broken with weight issue polkadot-js/apps#6349 (comment)

@zjb0807

This comment has been minimized.

@GopherJ
Copy link
Contributor Author

GopherJ commented Oct 21, 2021

pub struct GenesisConfig {
/// The default version to encode outgoing XCM messages with.
pub safe_xcm_version: Option<XcmVersion>,
}

pub struct GenesisConfig<T: Config> {
        /// The default version to encode outgoing XCM messages with.
        pub safe_xcm_version: Option<XcmVersion>,
        pub phantom: sp_std::marker::PhantomData<T>,
}

Can you change this? So can change it in TestExternalities

pallet_xcm::GenesisConfig::<Runtime> {
        safe_xcm_version: Some(2),
        ..Default::default()
}
.assimilate_storage(&mut t)
.unwrap();


.assimilate_storage(&mut t)
    |      ^^^^^^^^^^^^^^^^^^ cannot infer type for type parameter `T` declared on the trait `GenesisBuild`

can you review now to see if it's ok?

xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@zjb0807 zjb0807 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@zjb0807
Copy link
Contributor

zjb0807 commented Oct 21, 2021

    #[cfg(feature = "std")]
    impl GenesisConfig {
        /// Direct implementation of `GenesisBuild::build_storage`.
        ///
        /// Kept in order not to break dependency.
        pub fn build_storage<T: Config>(&self) -> Result<sp_runtime::Storage, String> {
            <Self as frame_support::traits::GenesisBuild<T>>::build_storage(self)
        }

        /// Direct implementation of `GenesisBuild::assimilate_storage`.
        ///
        /// Kept in order not to break dependency.
        pub fn assimilate_storage<T: Config>(
            &self,
            storage: &mut sp_runtime::Storage,
        ) -> Result<(), String> {
            <Self as frame_support::traits::GenesisBuild<T>>::assimilate_storage(self, storage)
        }
    }
    pallet_xcm::GenesisConfig { safe_xcm_version: Some(2) }
        .assimilate_storage::<Test>(&mut t)
        .unwrap();

The implementation of GenesisBuild::assimilate_storage avoide PhantomData.

@KiChjang KiChjang requested a review from gui1117 October 21, 2021 14:38
xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Can't genesis remain not generic?

@KiChjang
Copy link
Contributor

pub struct GenesisConfig {
/// The default version to encode outgoing XCM messages with.
pub safe_xcm_version: Option<XcmVersion>,
}

pub struct GenesisConfig<T: Config> {
        /// The default version to encode outgoing XCM messages with.
        pub safe_xcm_version: Option<XcmVersion>,
        pub phantom: sp_std::marker::PhantomData<T>,
}

Can you change this? So can change it in TestExternalities

pallet_xcm::GenesisConfig::<Runtime> {
        safe_xcm_version: Some(2),
        ..Default::default()
}
.assimilate_storage(&mut t)
.unwrap();


.assimilate_storage(&mut t)
    |      ^^^^^^^^^^^^^^^^^^ cannot infer type for type parameter `T` declared on the trait `GenesisBuild`

@zjb0807 I'm actually unsure why you're hitting this issue, but I don't think adding a type parameter to GenesisConfig solves your problem, in fact it looks to me like a hack if it did "accidentally" solve your issue.

@zjb0807
Copy link
Contributor

zjb0807 commented Oct 22, 2021

@zjb0807 I'm actually unsure why you're hitting this issue, but I don't think adding a type parameter to GenesisConfig solves your problem, in fact it looks to me like a hack if it did "accidentally" solve your issue.

I realized that this is not good, so I suggested this way again.

    #[cfg(feature = "std")]
    impl GenesisConfig {
        /// Direct implementation of `GenesisBuild::build_storage`.
        ///
        /// Kept in order not to break dependency.
        pub fn build_storage<T: Config>(&self) -> Result<sp_runtime::Storage, String> {
            <Self as frame_support::traits::GenesisBuild<T>>::build_storage(self)
        }

        /// Direct implementation of `GenesisBuild::assimilate_storage`.
        ///
        /// Kept in order not to break dependency.
        pub fn assimilate_storage<T: Config>(
            &self,
            storage: &mut sp_runtime::Storage,
        ) -> Result<(), String> {
            <Self as frame_support::traits::GenesisBuild<T>>::assimilate_storage(self, storage)
        }
    }
    pallet_xcm::GenesisConfig { safe_xcm_version: Some(2) }
        .assimilate_storage::<Test>(&mut t)
        .unwrap();

The implementation of GenesisBuild::assimilate_storage avoide PhantomData.

Copy link
Contributor

@KiChjang KiChjang left a comment

Choose a reason for hiding this comment

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

@GopherJ Can we revert the changes that made GenesisConfig generic?

@GopherJ GopherJ force-pushed the feat/export-xcm-pallet-config branch from 987b0da to 0c01dda Compare October 22, 2021 01:39
@GopherJ
Copy link
Contributor Author

GopherJ commented Oct 22, 2021

@GopherJ Can we revert the changes that made GenesisConfig generic?

done

@zjb0807
Copy link
Contributor

zjb0807 commented Oct 22, 2021

@GopherJ Can we revert the changes that made GenesisConfig generic?

done

Can you help add this implementation?

    #[cfg(feature = "std")]
    impl GenesisConfig {
        /// Direct implementation of `GenesisBuild::build_storage`.
        ///
        /// Kept in order not to break dependency.
        pub fn build_storage<T: Config>(&self) -> Result<sp_runtime::Storage, String> {
            <Self as frame_support::traits::GenesisBuild<T>>::build_storage(self)
        }

        /// Direct implementation of `GenesisBuild::assimilate_storage`.
        ///
        /// Kept in order not to break dependency.
        pub fn assimilate_storage<T: Config>(
            &self,
            storage: &mut sp_runtime::Storage,
        ) -> Result<(), String> {
            <Self as frame_support::traits::GenesisBuild<T>>::assimilate_storage(self, storage)
        }
    }
    pallet_xcm::GenesisConfig { safe_xcm_version: Some(2) }
        .assimilate_storage::<Test>(&mut t)
        .unwrap();

The implementation of GenesisBuild::assimilate_storage avoide PhantomData.

@KiChjang
Copy link
Contributor

@zjb0807 Sorry, what exactly is going on here? Are you asking for a technical support, or are you suggesting a change on this PR?

@zjb0807
Copy link
Contributor

zjb0807 commented Oct 22, 2021

@zjb0807 Sorry, what exactly is going on here? Are you asking for a technical support, or are you suggesting a change on this PR?

I want to modify the genesis config in TestExternalities.

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

@KiChjang
Copy link
Contributor

@zjb0807 And why are you asking about that here in this PR?

@zjb0807
Copy link
Contributor

zjb0807 commented Oct 22, 2021

@zjb0807 And why are you asking about that here in this PR?

Ok. You can approve the PR and I'll do it through another PR.

@KiChjang
Copy link
Contributor

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 84466fe into paritytech:master Oct 22, 2021
@GopherJ GopherJ deleted the feat/export-xcm-pallet-config branch October 22, 2021 02:26
@GopherJ
Copy link
Contributor Author

GopherJ commented Oct 22, 2021

@zjb0807 And why are you asking about that here in this PR?

Ok. You can approve the PR and I'll do it through another PR.

yes that'll maybe be easier because I didn't understand it yet sorry didn't help:)

ordian added a commit that referenced this pull request Oct 27, 2021
* master: (73 commits)
  Fix XCM Teleport Benchmark (#4146)
  Allow Queries and Subscriptions (#4150)
  Fix weights on hard-coded XCM fragments (#4144)
  Bump spec versions (#4142)
  Bump libc from 0.2.104 to 0.2.105 (#4141)
  Enable bags-list pallet in polkadot (#4080)
  Move artifacts states into memory in PVF validation host (#3907)
  Introduce new Runtime API endpoint for fetching the validation data (#3728)
  export xcm_pallet config (#4116)
  Apply cargo fmt (#4122)
  remove logging from the check-dependent-* job (#4120)
  Remove stale migrations (#4107)
  Bump structopt from 0.3.23 to 0.3.25 (#4098)
  remove hardcoded pipeline scripts tag (#4109)
  Bump libc from 0.2.103 to 0.2.104 (#4099)
  some spelling fixes (#4088)
  polkadot: remove call filters on registrar pallets (#4093)
  Fix typos in docs (#4092)
  Rename Statemint where appropriate (#4087)
  bump substrate (#4091)
  ...
emostov pushed a commit that referenced this pull request Nov 1, 2021
* export xcm_pallet config

Signed-off-by: Cheng JIANG <[email protected]>

* run format

Signed-off-by: Cheng JIANG <[email protected]>

* fix typo

Signed-off-by: Cheng JIANG <[email protected]>

* add generic parameter to support different runtimes

* Revert "add generic parameter to support different runtimes"

This reverts commit 4405ea9.
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. 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.

export xcm_pallet config so that we can set SafeXcmVersion
5 participants