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.

Sign up for GitHub

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
Merged

Remove bounds from PrevalidateAttests struct definition #2886

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec26d46
Make PrevalidateAttests Send + Sync and remove bounds
ggwpez Jan 9, 2024
7cb1ed6
Add prdoc
ggwpez Jan 9, 2024
cfaf278
Update polkadot/runtime/common/src/claims.rs
ggwpez Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions polkadot/runtime/common/src/claims.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,11 +591,9 @@ impl<T: Config> Pallet<T> {
/// otherwise free to place on chain.
#[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)]
#[scale_info(skip_type_params(T))]
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 •
edited
Loading

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.


impl<T: Config + Send + Sync> Debug for PrevalidateAttests<T>
impl<T: Config> Debug for PrevalidateAttests<T>
where
<T as frame_system::Config>::RuntimeCall: IsSubType<Call<T>>,
{
Expand All @@ -610,7 +608,7 @@ where
}
}

impl<T: Config + Send + Sync> PrevalidateAttests<T>
impl<T: Config> PrevalidateAttests<T>
where
<T as frame_system::Config>::RuntimeCall: IsSubType<Call<T>>,
{
Expand All @@ -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.

where
<T as frame_system::Config>::RuntimeCall: IsSubType<Call<T>>,
{
Expand Down Expand Up @@ -1469,6 +1467,12 @@ mod tests {
);
});
}

#[test]
fn prevalidate_assets_is_send_and_sync() {
fn is_send_and_sync<T: Send + Sync>() {}
is_send_and_sync::<PrevalidateAttests<Test>>();
}
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
13 changes: 13 additions & 0 deletions prdoc/pr_2886.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Remove bounds from `PrevalidateAttests` struct definition

doc:
- audience: Runtime Dev
description: |
Minimal change to `PrevalidateAssets` to remove some trait bounds on the struct itself while
keeping all its capabilities.

crates:
- name: polkadot-runtime-common