Update pallet-nis to support Block Number Provider #6764
Conversation
|
Hello @gui1117, please review |
| @@ -312,6 +317,8 @@ pub mod pallet { | |||
| #[pallet::constant] | |||
| type ThawThrottle: Get<(Perquintill, BlockNumberFor<Self>)>; | |||
|
|
|||
| type BlockNumberProvider: BlockNumberProvider<BlockNumber: Default>; | |||
| @@ -693,7 +702,7 @@ pub mod pallet { | |||
| let (owner, mut on_hold) = receipt.owner.ok_or(Error::<T>::AlreadyCommunal)?; | |||
| ensure!(owner == who, Error::<T>::NotOwner); | |||
|
|
|||
| let now = frame_system::Pallet::<T>::block_number(); | |||
| let now = T::BlockNumberProvider::current_block_number(); | |||
There was a problem hiding this comment.
Misses a migration for the Receipts to change the expiry to the "new clock". (Not required when switched to System as block number provider.
There was a problem hiding this comment.
Most of those PR is for pallet in the relay chain to go to a parachain without requiring migration.
But in the wider scope I can see parachain could also make use of the relay chain block number provider when they are going more agile and skip some blocks.
In this case a migration would help indeed. We can just provide a function helper to do this migration, no need for storage versioning. Because parachain could change the configuration of the block number whenever they want, so unrelated to the version of the pallet.
@kianenigma do you think we should provide the block number migration function for all such pallets? I think some approved one don't have it.
Or should it be done on-demand when people ask for it?
There was a problem hiding this comment.
I would not include a migration for the time being, assuming the PR and code-doc are super clear under which circumstances you do, and do not need a migration.
There was a problem hiding this comment.
But in the wider scope I can see parachain could also make use of the relay chain block number provider when they are going more agile and skip some blocks.
This is the entire point of this exercise. Otherwise it is quite useless. Not sure why we approve prs without a migration. Also requiring everyone to write their own migration is a little bit stupid. Especially as by just "reading the docs" it will not be that easy. Block numbers are stored internally in some structures and may not be that obvious for others to find them etc. This will just end up in a mess, if people are forced to write their own migrations.
| ); | ||
| Nis::on_initialize( | ||
| <Test as pallet_nis::Config>::BlockNumberProvider::current_block_number(), | ||
| ); |
There was a problem hiding this comment.
on_finalize/on_initialize is taking system block number. Here is works because both pallet_nis block number and system block number are the same.
I think the code is cleaner if we use system block number same as before in this test no?
Part of #6297
Review Notes
ReceiptRecord&SummaryRecordtypes used in theReceiptsandSummarystorage items respectively, now adoptsT::BlockNumberProviderwith the condition that concrete types implementDefault