Update Pallet Referenda to support Block Number Provider #6338
Conversation
…da_block_number_provider
…da_block_number_provider
|
can you help review this again @gui1117 |
|
@dharjeezy this is nearly done, will you take it over the finish line? |
i have updated the PR with the migration written. @acatangiu @bkchr |
substrate/frame/referenda/src/lib.rs
Outdated
| RuntimeOrigin = <Self::RuntimeOrigin as OriginTrait>::PalletsOrigin, | ||
| >; | ||
|
|
||
| /// The preimage provider. | ||
| type Preimages: QueryPreimage<H = Self::Hashing> + StorePreimage; | ||
|
|
||
| /// Provider for the block number. Normally this is the `frame_system` pallet. |
There was a problem hiding this comment.
| /// Provider for the block number. Normally this is the `frame_system` pallet. | |
| /// Provider for the block number. | |
| /// | |
| /// Normally this is the `frame_system` pallet. |
| @@ -161,6 +178,98 @@ pub mod v1 { | |||
| } | |||
| } | |||
|
|
|||
| pub mod v2 { | |||
There was a problem hiding this comment.
| pub mod v2 { | |
| /// Migration for when changing the block number provider. | |
| pub mod switch_block_number_provider { |
| use super::*; | ||
|
|
||
| /// The log target. | ||
| const TARGET: &'static str = "runtime::referenda::migration::v2"; |
There was a problem hiding this comment.
| const TARGET: &'static str = "runtime::referenda::migration::v2"; | |
| const TARGET: &'static str = "runtime::referenda::migration::change_block_number_provider"; |
| pub trait BlockToRelayHeightConversion<T: Config<I>, I: 'static> { | ||
| fn convert_block_number_to_relay_height( | ||
| block_number: SystemBlockNumberFor<T>, | ||
| ) -> BlockNumberFor<T, I>; | ||
| } |
There was a problem hiding this comment.
| pub trait BlockToRelayHeightConversion<T: Config<I>, I: 'static> { | |
| fn convert_block_number_to_relay_height( | |
| block_number: SystemBlockNumberFor<T>, | |
| ) -> BlockNumberFor<T, I>; | |
| } | |
| /// Convert from one to another block number provider/type. | |
| pub trait BlockNumberConversion<Old, New> { | |
| /// Convert the `old` block number type to the new block number type. | |
| /// | |
| /// Any changes in the rate of blocks need to be taken into account. | |
| fn convert_block_number( | |
| block_number: Old, | |
| ) -> New; | |
| } |
There was a problem hiding this comment.
this is not addressed. @bkchr
| if on_chain_version != 1 { | ||
| log::warn!(target: TARGET, "skipping migration from v1 to v2."); | ||
| return weight | ||
| } |
There was a problem hiding this comment.
| if on_chain_version != 1 { | |
| log::warn!(target: TARGET, "skipping migration from v1 to v2."); | |
| return weight | |
| } |
Please add a comment to the migration that this is not guarded.
| } | ||
|
|
||
| /// Transforms SystemBlockNumberFor<T> to BlockNumberFor<T,I> | ||
| pub struct MigrateV1ToV2<BlockConversion, T, I = ()>( |
There was a problem hiding this comment.
IMO this should probably just be a standalone function. Downstream people need to write their own migration with some custom guarding or whatever to ensure that they don't apply it multiple times.
|
you can see the failing jobs on the top with the new github merge experience, see at the bottom of the page. Then you can see rustdoc build is failing. |
| pub(crate) type ReferendumInfoOf<T, I> = ReferendumInfo< | ||
| TrackIdOf<T, I>, | ||
| PalletsOriginOf<T>, | ||
| SystemBlockNumberFor<T>, | ||
| BoundedCallOf<T, I>, | ||
| BalanceOf<T, I>, | ||
| TallyOf<T, I>, | ||
| <T as frame_system::Config>::AccountId, | ||
| ScheduleAddressOf<T, I>, | ||
| >; | ||
|
|
||
| #[storage_alias] | ||
| pub type ReferendumInfoFor<T: Config<I>, I: 'static> = | ||
| StorageMap<Pallet<T, I>, Blake2_128Concat, ReferendumIndex, ReferendumInfoOf<T, I>>; |
There was a problem hiding this comment.
Personally I would move it to the other module. And consider the V1 storage to be with the provided block number.
There was a problem hiding this comment.
But why? isn't the reason why this was declared here is to tell that this Type and Storage was supported in v1?
There was a problem hiding this comment.
let's consider we implement this as storage version 2.
I have my pallet in my chain in storage version 1.
I update the version of my pallet without changing the block number provider, the migration executes don't change the storage and write storage version 2.
Then I want to change the BlockNumberProvider.
I can't reuse the logic, the pallet is already in version 2.
There was a problem hiding this comment.
but maybe it is better to keep this storage here.
If the type BlockNumber is changing, they would want to do the migration v0 -> v1 -> migrate_block_number_provider
If we change the type here to the provided block number then they will have to write a manual migration.
Maybe you were right when implementing with v2. And we should have both a v1 to v2 migration and a migrate_block_number_provider standalone migration.
But because everybody use u32 for block number I think having v0 -> v1 -> migrate_block_number_provider is ok.
In conclusion: I think it is good as it is.
|
|
||
| impl BlockNumberConversion<SystemBlockNumberFor<T>, BlockNumberFor<T, ()>> for MockBlockConverter { | ||
| fn convert_block_number(block_number: SystemBlockNumberFor<T>) -> BlockNumberFor<T, ()> { | ||
| block_number as u64 |
There was a problem hiding this comment.
Better to make it not constant ? add a delay like block_number + 10
| pub struct MockBlockConverter; | ||
|
|
||
| impl BlockNumberConversion<SystemBlockNumberFor<T>, BlockNumberFor<T, ()>> for MockBlockConverter { | ||
| fn convert_block_number(block_number: SystemBlockNumberFor<T>) -> BlockNumberFor<T, ()> { |
There was a problem hiding this comment.
In the mock both are System but I think it is ok.
|
CI is failing because some unused variable |
| doc: | ||
| - audience: Runtime Dev | ||
| description: | | ||
| This PR makes the referenda pallet uses the relay chain as a block provider for a parachain on a regular schedule. |
There was a problem hiding this comment.
not sure what you mean with this sentence. the PR provides a new configuration
|
@dharjeezy please check this comment - #6297 (comment) |
This PR introduces BlockNumberProvider config for the referenda pallet.
closes part of #6297
Polkadot address: 12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW