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

Remove bounds from PrevalidateAttests struct definition #2886

Merged
merged 3 commits into from
Jan 9, 2024

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jan 9, 2024

Removing some bounds as it came up in #2726 while still keeping Send + Sync capabilities.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added the T8-polkadot This PR/Issue is related to/affects the Polkadot network. label Jan 9, 2024
@ggwpez ggwpez changed the title Make PrevalidateAttests Send + Sync and remove bounds Remove bounds from PrevalidateAttests struct definition Jan 9, 2024
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
pub struct PrevalidateAttests<T: Config + Send + Sync>(sp_std::marker::PhantomData<T>)
where
<T as frame_system::Config>::RuntimeCall: IsSubType<Call<T>>;
pub struct PrevalidateAttests<T>(core::marker::PhantomData<fn(T)>);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we really need this :P

Copy link
Member Author

@ggwpez ggwpez Jan 9, 2024

Choose a reason for hiding this comment

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

WDYM? The PhantomData<fn(T)>?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

But if i use PhantomData<T> then its not Send + Sync and if i remove the PD then it errors parameter T is never used.
Signed Extension does require it though.

Copy link
Member

Choose a reason for hiding this comment

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

Why does it needs to be Send and Sync?

Copy link
Member Author

Choose a reason for hiding this comment

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

The SignedExtension trait requires it, i did not check why.
The new TEs will also require it.

@@ -620,7 +618,7 @@ where
}
}

impl<T: Config + Send + Sync> SignedExtension for PrevalidateAttests<T>
impl<T: Config> SignedExtension for PrevalidateAttests<T>
Copy link
Member

Choose a reason for hiding this comment

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

If you would just kept the bounds here, it would have been enough.

But we can go with the phantomdata trick.

@bkchr bkchr added the R0-silent Changes should not be mentioned in any release notes label Jan 9, 2024
Co-authored-by: Bastian Köcher <git@kchr.de>
@ggwpez ggwpez enabled auto-merge (squash) January 9, 2024 17:16
@ggwpez ggwpez merged commit 49cea35 into master Jan 9, 2024
119 of 121 checks passed
@ggwpez ggwpez deleted the oty-prevalidate-assets branch January 9, 2024 17:44
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 T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants