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

Removes constraint in BlockNumberProvider from treasury #6522

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

Conversation

gupnik
Copy link
Contributor

@gupnik gupnik commented Nov 18, 2024

#3970 updated the treasury pallet to support relay chain block number provider. However, it added a constraint to the BlockNumberProvider to have the same block number type as frame_system:

type BlockNumberProvider: BlockNumberProvider<BlockNumber = BlockNumberFor<Self>>;

This PR removes that constraint as suggested by @gui1117

@gupnik gupnik added the T2-pallets This PR/Issue is related to a particular pallet. label Nov 18, 2024
@gupnik gupnik requested a review from a team as a code owner November 18, 2024 12:20
@@ -106,7 +106,7 @@ use frame_support::{
weights::Weight,
BoundedVec, PalletId,
};
use frame_system::pallet_prelude::BlockNumberFor;
use frame_system::pallet_prelude::BlockNumberFor as SystemBlockNumberFor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to #6245, someday this type alias can be removed to avoid confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

May be local block number? What system really means here. Also BlockNumberFor reference to the block's header, not system pallet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be local block number? What system really means here. Also BlockNumberFor reference to the block's header, not system pallet

Since we are importing it from frame_system's prelude, I believe that SystemBlockNumberFor is reasonable.

@@ -122,6 +122,8 @@ pub type NegativeImbalanceOf<T, I = ()> = <<T as Config<I>>::Currency as Currenc
>>::NegativeImbalance;
type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;
type BeneficiaryLookupOf<T, I> = <<T as Config<I>>::BeneficiaryLookup as StaticLookup>::Source;
pub type BlockNumberFor<T, I = ()> =
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather come up with new name, and not reuse the one which is used for local block number type

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 would rather come up with new name, and not reuse the one which is used for local block number type

I am actually doing it the other way around calling the local one as SystemBlockNumberFor.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes but this forces you to not import use frame_system::pallet_prelude::*;

I think having a different name that doesn't conflict with frame_system::BlockNumberFor would be a bit cleaner.

maybe like ProvidedBlockNumberFor? I agree the name is not good either.

anyway everything is good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes but this forces you to not import use frame_system::pallet_prelude::*;

Yeah, my hope is to be able to provide a different type in the prelude once enough pallets have been migrated.

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/11930011112
Failed job name: test-linux-stable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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