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

Migrate pallet-statement and pallet-sudo to use umbrella crate #7106

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

Conversation

Nathy-bajo
Copy link

@Nathy-bajo Nathy-bajo commented Jan 9, 2025

Part of #6504

Polkadot Address: 121HJWZtD13GJQPD82oEj3gSeHqsRYm1mFgRALu4L96kfPD1

@Nathy-bajo Nathy-bajo requested a review from a team as a code owner January 9, 2025 20:29
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Jan 9, 2025

User @Nathy-bajo, please sign the CLA here.

@Nathy-bajo
Copy link
Author

Please review @re-gius @kianenigma

@@ -0,0 +1,21 @@
thread 'coordinator' panicked at /rustc/f7273e0044ad8f35ad27282e4ab776af50b61a54/compiler/rustc_codegen_ssa/src/back/write.rs:1662:29:
Copy link
Contributor

Choose a reason for hiding this comment

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

wtf :D

use sp_core::Pair;
use sp_runtime::{traits::IdentityLookup, AccountId32, BuildStorage};
use frame::testing_prelude::*;
use frame::deps::{sp_runtime::{traits::IdentityLookup, AccountId32, BuildStorage}, sp_core::Pair};
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of this stuff should already be testing prelude.

use sp_core::Pair;
use sp_runtime::AccountId32;
use sp_statement_store::{
use frame::deps::{sp_runtime::AccountId32, sp_core::{Pair, H256}, sp_statement_store::{
Copy link
Contributor

Choose a reason for hiding this comment

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

same.

@@ -569,7 +569,8 @@ pub mod deps {
pub use sp_core;
pub use sp_io;
pub use sp_runtime;

pub use sp_statement_store;
Copy link
Contributor

Choose a reason for hiding this comment

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

All of the primitives of the statemnt store are really just used in one pallet, I would not add them here and keep them as a dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah

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.

not ready yet :)

@Nathy-bajo Nathy-bajo changed the title Migrate pallet-statement to use umbrella crate Migrate pallet-statement and pallet-sudo to use umbrella crate Jan 12, 2025
@Nathy-bajo
Copy link
Author

Please review @bkchr @kianenigma @re-gius

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Yeah statement-store primitives you not be added to the frame crate

@@ -569,7 +569,8 @@ pub mod deps {
pub use sp_core;
pub use sp_io;
pub use sp_runtime;

pub use sp_statement_store;
Copy link
Member

Choose a reason for hiding this comment

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

Yeah

@paritytech-review-bot paritytech-review-bot bot requested a review from a team January 14, 2025 22:58
@Nathy-bajo
Copy link
Author

Yeah statement-store primitives you not be added to the frame crate

I thought I already removed it. Thank you for the reference!

substrate/frame/Cargo.toml Outdated Show resolved Hide resolved
substrate/frame/src/lib.rs Outdated Show resolved Hide resolved
@bkchr bkchr enabled auto-merge January 15, 2025 09:45
@bkchr bkchr requested a review from kianenigma January 15, 2025 09:45
@bkchr bkchr added the T2-pallets This PR/Issue is related to a particular pallet. label Jan 15, 2025
@bkchr bkchr added the R0-silent Changes should not be mentioned in any release notes label Jan 15, 2025
substrate/frame/sudo/src/benchmarking.rs Outdated Show resolved Hide resolved
substrate/frame/sudo/src/mock.rs Outdated Show resolved Hide resolved
substrate/frame/sudo/src/weights.rs Outdated Show resolved Hide resolved
auto-merge was automatically disabled January 16, 2025 09:05

Head branch was pushed to by a user without write access

@github-actions github-actions bot requested review from bkchr and gui1117 January 16, 2025 09:05
Copy link
Contributor

Review required! Latest push from author must always be reviewed

InvalidTransaction, TransactionPriority, TransactionValidityError, UnknownTransaction,
ValidTransaction,
},
DispatchResult,
};
/// Other error/result types for runtime
#[doc(no_inline)]
Copy link
Member

Choose a reason for hiding this comment

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

Can you please move the import from below to the other import above.

Copy link
Author

Choose a reason for hiding this comment

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

It was the wrong import

@Nathy-bajo
Copy link
Author

Please review @re-gius @bkchr

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 T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants