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.
There was a problem hiding this comment.
Gone ahead writing a migration
substrate/frame/nis/src/mock.rs
Outdated
| ); | ||
| 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?
There was a problem hiding this comment.
Thanks for the review
| pub type BlockNumberFor<T> = | ||
| <<T as Config>::BlockNumberProvider as BlockNumberProvider>::BlockNumber; | ||
| pub type BalanceOf<T> = |
There was a problem hiding this comment.
I don't mind reexporting types, but those names are so common I am afraid it will crate unecessary conflicts and bad IDE suggestions.
| pub type BlockNumberFor<T> = | |
| <<T as Config>::BlockNumberProvider as BlockNumberProvider>::BlockNumber; | |
| pub type BalanceOf<T> = | |
| pub(crate) type BlockNumberFor<T> = | |
| <<T as Config>::BlockNumberProvider as BlockNumberProvider>::BlockNumber; | |
| pub(crate) type BalanceOf<T> = |
| <<T as Config>::Currency as FunInspect<<T as frame_system::Config>::AccountId>>::Balance; | ||
| type DebtOf<T> = | ||
| fungible::Debt<<T as frame_system::Config>::AccountId, <T as Config>::Currency>; | ||
| type ReceiptRecordOf<T> = | ||
| pub type ReceiptRecordOf<T> = |
There was a problem hiding this comment.
| pub type ReceiptRecordOf<T> = | |
| pub(crate) type ReceiptRecordOf<T> = |
| <T as frame_system::Config>::AccountId, | ||
| SystemBlockNumberFor<T>, | ||
| BalanceOf<T>, | ||
| >; |
There was a problem hiding this comment.
we would also need to migrate Summary storage.
| // Iterate over v0::Receipts and update the expiry in Receipts | ||
| v0::Receipts::<T>::iter().for_each(|(index, v0_receipt)| { | ||
| // Use mutate to modify the value directly in the storage | ||
| Receipts::<T>::mutate(index, |maybe_receipt| { |
There was a problem hiding this comment.
Doesn't look good, Here we are decoding the value with the old type. Note that system block number and relay block number can be different types like u32 and u64.
We should only write the new value.
Part of #6297
Review Notes
ReceiptRecord&SummaryRecordtypes used in theReceiptsandSummarystorage items respectively, now adoptsT::BlockNumberProviderwith the condition that concrete types implementDefault