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

staging-xcm: Use versioned_type! for VersionedXcm #4378

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dastansam
Copy link
Contributor

closes #3846

@dastansam dastansam requested a review from a team as a code owner May 4, 2024 13:08
@dastansam dastansam changed the title Use versioned_type! for VersionedXcm staging-xcm: Use versioned_type! for VersionedXcm May 4, 2024
@dastansam dastansam marked this pull request as draft May 4, 2024 13:44
@dastansam dastansam marked this pull request as ready for review May 4, 2024 23:34
VersionedXcm::V2(x)
}
}

impl<RuntimeCall> From<v3::Xcm<RuntimeCall>> for VersionedXcm<RuntimeCall> {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this staying?

polkadot/xcm/src/lib.rs Outdated Show resolved Hide resolved
polkadot/xcm/src/lib.rs Outdated Show resolved Hide resolved
@@ -77,6 +77,37 @@ pub trait TryAs<T> {
}

macro_rules! versioned_type {
(@internal $n:ident, $v3:ty, ) => {
// only impl `MaxEncodedLen` for enums without generic type parameters
Copy link
Member

Choose a reason for hiding this comment

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

It makes no sense that this is separate.

Copy link
Contributor Author

@dastansam dastansam May 5, 2024

Choose a reason for hiding this comment

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

MaxEncodedLen is not implemented for Xcm structs of VersionedXcm, so impl it fails. This is a workaround I did to exclude VersionedXcm from it implementing MaxEncodedLen. Open to suggestions for a more idiomatic way of handling it

// only impl `MaxEncodedLen` for enums without generic type parameters
impl MaxEncodedLen for $n {
fn max_encoded_len() -> usize {
<$v3>::max_encoded_len()
Copy link
Member

Choose a reason for hiding this comment

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

This should be the max of all variants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some enums' 2nd versions do not implement MaxEncodedLen, v3 and v4 all implement it, so I am now taking max of v3 and v4

@dastansam dastansam requested a review from bkchr May 5, 2024 23:40
@franciscoaguirre franciscoaguirre added the T6-XCM This PR/Issue is related to XCM. label May 8, 2024
@bkchr bkchr added the R0-silent Changes should not be mentioned in any release notes label May 13, 2024
prdoc/pr_4378.prdoc Outdated Show resolved Hide resolved
prdoc/pr_4378.prdoc Outdated Show resolved Hide resolved
prdoc/pr_4378.prdoc Outdated Show resolved Hide resolved
@acatangiu
Copy link
Contributor

Hope @franciscoaguirre can help with the macro review, I got lost there 🙈

@bkchr
Copy link
Member

bkchr commented Jul 24, 2024

@dastansam could you fix the CI issues?

#[scale_info(replace_segment("staging_xcm", "xcm"))]
pub enum VersionedXcm<RuntimeCall> {
#[codec(index = 2)]
#[deprecated]
Copy link
Contributor

Choose a reason for hiding this comment

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

this deprecated seems removed by the PR.

}
impl MaxEncodedLen for $n {
fn max_encoded_len() -> usize {
<$v3>::max_encoded_len().max(<$v4>::max_encoded_len())
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can also fix for the other implementation of MaxEncodedLen when there is only v3 and v4:
https://github.com/paritytech/polkadot-sdk/pull/4378/files#diff-8eb727f0e7d0dd42a8d45ab05f9450777ed6af6d22021b81d0c449e8a32aba78R200

@dastansam
Copy link
Contributor Author

dastansam commented Jul 27, 2024

@dastansam could you fix the CI issues?

I also got lost in the macros, as it's been couple of months since I worked on it. I am getting familiar with it again and will also address PR comments from @gui1117

franciscoaguirre and others added 2 commits August 5, 2024 09:55
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

Looks good! We probably only need to make the CI happy in order to merge it

- audience: Runtime Dev
description: |
Currently, all other versioned types in the `staging-xcm` are created using `versioned_type!` macro, except for `VersionedXcm`.
This PR changes `versioned_type` macro so that it can be used for `VersionedXcm` as well. This is done by adding optional
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This PR changes `versioned_type` macro so that it can be used for `VersionedXcm` as well. This is done by adding optional
This PR changes `versioned_type` macro so that it can be used for `VersionedXcm` as well, reducing a lot of duplication. This is done by adding optional

@@ -476,66 +469,6 @@ impl<C> VersionedXcm<C> {
}
}

impl<RuntimeCall> From<v2::Xcm<RuntimeCall>> for VersionedXcm<RuntimeCall> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Beautiful

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6896246

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VersionedXcm is not created with versioned_type
6 participants