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

Use frame umbrella crate in pallet-proxy and pallet-multisig #5995

Merged
merged 44 commits into from
Oct 29, 2024

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Oct 9, 2024

A step towards #4782

In order to nail down the right preludes in polkadot-sdk-frame, we need to migrate a number of pallets to be written with it. Moreover, migrating our pallets to this simpler patter will encourage the ecosystem to also follow along.

If this PR is approved and has no unwanted negative consequences, I will make a tracking issue to migrate all pallets to this umbrella crate.

TODO:

  • fix frame benchmarking template. Can we detect the umbrella crate in there and have an if else? cc @ggwpez
  • Migrate benchmarking to v2 @re-gius a good candidate for you, you can open a PR against my branch.
  • tracking issue with follow-ups

@kianenigma kianenigma requested a review from a team as a code owner October 9, 2024 13:26
@kianenigma kianenigma added R0-silent Changes should not be mentioned in any release notes T1-FRAME This PR/Issue is related to core FRAME, the framework. and removed R0-silent Changes should not be mentioned in any release notes labels Oct 9, 2024
substrate/frame/Cargo.toml Outdated Show resolved Hide resolved
frame-system = { workspace = true }
sp-io = { workspace = true }
sp-runtime = { workspace = true }
frame = { workspace = true, features = ["experimental", "runtime"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

do we actually need experimental? I don't see it used in the past AFAICT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove it as soon as I am a bit more confident in the readiness of the frame crate 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What we should try here is to see if removing the feature = runtime still works?

The idea in the frame umbrella crate was that for pallet development, you would only need crate itself, and for a runtime, the crate plus runtime feature.

@@ -42,15 +36,14 @@ pub mod v1 {
pub struct MigrateToV1<T>(core::marker::PhantomData<T>);
impl<T: Config> OnRuntimeUpgrade for MigrateToV1<T> {
#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
fn pre_upgrade() -> Result<Vec<u8>, frame::deps::sp_runtime::TryRuntimeError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to the PR, but maybe we could introduce: TryRuntimeResult<T> = Result<T, TryRuntimeError>

substrate/frame/multisig/src/migrations.rs Outdated Show resolved Hide resolved
substrate/frame/multisig/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/proxy/Cargo.toml Show resolved Hide resolved
substrate/frame/src/lib.rs Outdated Show resolved Hide resolved
templates/minimal/runtime/Cargo.toml Outdated Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team October 15, 2024 14:58
Comment on lines 8 to 9
#[allow(unused_imports)]
use frame::prelude::*;
Copy link
Member

Choose a reason for hiding this comment

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

why though? just for the template to be extended? perhaps you are better to find some way to just use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I was trying to keep this minimal.

@re-gius can you add one u32 storage value in this crate to fix this? then #[allow(unused_imports)] can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added in 5918bef

use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}};
use core::marker::PhantomData;
use frame::weights_prelude::*;
Copy link
Member

Choose a reason for hiding this comment

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

this change doesn't make sense. it needs to be done at the template level, or handled later.

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

went through all the files, and changes look good except the benchmarks which need template level changes

re-gius and others added 3 commits October 15, 2024 17:32
Sub-task of PR #5995

## Integration

These changes should **not** require any integration effort.

---------

Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
@kianenigma kianenigma requested review from a team as code owners October 15, 2024 17:13
@paritytech-review-bot paritytech-review-bot bot requested a review from a team October 16, 2024 13:35
@@ -13,7 +13,6 @@ publish = false
codec = { workspace = true }
scale-info = { workspace = true }
polkadot-sdk = { workspace = true, features = [
"experimental",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it works without this?

Need to figure out the relationship between polkadot-sdk parent umbrella, and how it can be used to pull in polkadot-sdk-frame only. I thought experimental was needed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

build works, I don't know if there's anything else to check

@kianenigma
Copy link
Contributor Author

Feel free to approve this on my behalf once all open converstaions are serviced one last time :)

Copy link
Contributor

@re-gius re-gius left a comment

Choose a reason for hiding this comment

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

Approving on behalf of @kianenigma

@kianenigma
Copy link
Contributor Author

@pepoviola similar flaky ZN tests failing here.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Approving for Guiseppe.

@re-gius
Copy link
Contributor

re-gius commented Oct 29, 2024

Solves #6160

@re-gius re-gius added this pull request to the merge queue Oct 29, 2024
Merged via the queue into master with commit 9584dbd Oct 29, 2024
181 of 194 checks passed
@re-gius re-gius deleted the kiz-frame-umbrella-in-pallets branch October 29, 2024 10:42
mordamax pushed a commit to paritytech-stg/polkadot-sdk that referenced this pull request Oct 29, 2024
…itytech#5995)

A step towards paritytech#4782

In order to nail down the right preludes in `polkadot-sdk-frame`, we
need to migrate a number of pallets to be written with it. Moreover,
migrating our pallets to this simpler patter will encourage the
ecosystem to also follow along.

If this PR is approved and has no unwanted negative consequences, I will
make a tracking issue to migrate all pallets to this umbrella crate.

TODO: 

- [x] fix frame benchmarking template. Can we detect the umbrella crate
in there and have an `if else`? cc @ggwpez
- [x] Migrate benchmarking to v2 @re-gius a good candidate for you, you
can open a PR against my branch.
- [x] tracking issue with follow-ups

---------

Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: Giuseppe Re <[email protected]>
Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
mordamax pushed a commit to paritytech-stg/polkadot-sdk that referenced this pull request Oct 29, 2024
…itytech#5995)

A step towards paritytech#4782

In order to nail down the right preludes in `polkadot-sdk-frame`, we
need to migrate a number of pallets to be written with it. Moreover,
migrating our pallets to this simpler patter will encourage the
ecosystem to also follow along.

If this PR is approved and has no unwanted negative consequences, I will
make a tracking issue to migrate all pallets to this umbrella crate.

TODO: 

- [x] fix frame benchmarking template. Can we detect the umbrella crate
in there and have an `if else`? cc @ggwpez
- [x] Migrate benchmarking to v2 @re-gius a good candidate for you, you
can open a PR against my branch.
- [x] tracking issue with follow-ups

---------

Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: Giuseppe Re <[email protected]>
Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants