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-tips to umbrella crate #6532

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

Conversation

bennethxyz
Copy link

@bennethxyz bennethxyz commented Nov 18, 2024

part of #6504

Description

Modifying pallet-tips to use polkadot-frame-sdk re-exports and adding a few I think relevant for other pallets.

Review Notes

  • runtime::prelude now includes PalletId, I saw many other pallets use this in mock environments to distinguish instances of the same pallet and also identifiers for XCM.

  • Added ensure to top level prelude as it is also used in mock runtimes and other files across FRAME.

polkadot address: 15rM9idTv1izwNFqPabfcT4jkqHDsEvHSa5fASAUZwZcS4Yw

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Nov 18, 2024

User @bennethxyz, please sign the CLA here.

@bennethxyz bennethxyz marked this pull request as ready for review December 5, 2024 20:19
@bennethxyz bennethxyz requested a review from a team as a code owner December 5, 2024 20:19
@bennethxyz
Copy link
Author

@kianenigma @re-gius please review

@re-gius re-gius added R0-silent Changes should not be mentioned in any release notes T1-FRAME This PR/Issue is related to core FRAME, the framework. labels Dec 9, 2024
@bennethxyz
Copy link
Author

please review @re-gius

substrate/frame/src/lib.rs Outdated Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team January 4, 2025 10:26
substrate/frame/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/src/lib.rs Outdated Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team January 13, 2025 13:40
@@ -654,7 +639,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
/// 1. The number of entries in `Tips` should be equal to `Reasons`.
/// 2. Reasons exists for each Tip[`OpenTip.reason`].
/// 3. If `OpenTip.finders_fee` is true, then OpenTip.deposit should be greater than zero.
#[cfg(any(feature = "try-runtime", test))]
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why is this changed?

Copy link
Author

Choose a reason for hiding this comment

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

mainly because of this

error[E0433]: failed to resolve: could not find `try_runtime` in `frame`
   --> /polkadot-sdk/substrate/frame/tips/src/lib.rs:640:45
    |
640 |     pub fn do_try_state() -> Result<(), frame::try_runtime::TryRuntimeError> {
    |                                                ^^^^^^^^^^^ could not find `try_runtime` in `frame`
    |
note: found an item that was configured out
   --> /polkadot-sdk/substrate/frame/src/lib.rs:254:9
    |
254 | pub mod try_runtime {
    |         ^^^^^^^^^^^
note: the item is gated here
   --> /polkadot-sdk/substrate/frame/src/lib.rs:253:1
    |
253 | #[cfg(any(feature = "try-runtime", test))]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

<Test as crate::Config>::TipReportDepositBase::get() +
<Test as crate::Config>::DataDepositPerByte::get() *
<<Test as crate::Config>::TipReportDepositBase as TypedGet>::get() +
<<Test as crate::Config>::DataDepositPerByte as TypedGet>::get() *
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is TypedGet not in the prelude? I guess because otherwise get is conflicting. Then it is good as it is.

Copy link
Author

Choose a reason for hiding this comment

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

yeah

},
weights::Weight,
};
use core::str;

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

@@ -195,6 +181,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
pub fn build_and_execute(test: impl FnOnce() -> ()) {
new_test_ext().execute_with(|| {
test();
#[cfg(feature = "try-runtime")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it was better before when the state was asserted in the test.

@@ -631,10 +608,9 @@ fn report_awesome_and_tip_works_second_instance() {
}

#[test]
#[cfg(feature = "try-runtime")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I also prefered before

);
})
}

#[test]
#[cfg(feature = "try-runtime")]
Copy link
Contributor

Choose a reason for hiding this comment

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

same

);
})
}

#[test]
#[cfg(feature = "try-runtime")]
Copy link
Contributor

Choose a reason for hiding this comment

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

same

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 T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants