Add BlockNumberProvider to salary pallet config #7000
Conversation
PolkadotDom
changed the title
cumulus/parachains/runtimes/collectives/collectives-westend/Cargo.toml
Outdated
Show resolved
Hide resolved
| /// local moment in time (system block number) | ||
| fn equivalent_moment_in_time(local: SalaryLocalBlockNumber) -> SalaryNewBlockNumber { | ||
| let block_number = System::block_number(); | ||
| let local_duration = block_number.saturating_sub(local); |
There was a problem hiding this comment.
same maybe better to make it general if local > block_number, to be safe if this code gets copied and pasted.
| where | ||
| BC: ConvertBlockNumber<LocalBlockNumberFor<T>, NewBlockNumberFor<T, I>>, | ||
| { | ||
| fn on_runtime_upgrade() -> frame_support::weights::Weight { |
There was a problem hiding this comment.
could be better to implement some pre_upgrade and post_upgrade ensuring that Status value is some if it was some, and claimant count is same before and after the migration.
There was a problem hiding this comment.
Added. Though out of curiosity, is it possible for this not to be the case given the on_runtime_upgrade code? May be missing something about rust or substrate here.
cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs
Outdated
Show resolved
Hide resolved
| @@ -177,7 +177,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { | |||
| // implementation changes and behavior does not, then leave spec_version as | |||
| // is and increment impl_version. | |||
| spec_version: 268, | |||
| impl_version: 0, | |||
| impl_version: 1, | |||
There was a problem hiding this comment.
| impl_version: 1, | |
| impl_version: 0, |
The kitchensink runtime needs to bumping.
There was a problem hiding this comment.
The kitchensink runtime needs to bumping.
Assuming 'no' bumping was meant - is this because its also bumped automatically on release like the spec_version or because the change to the runtime was sufficient to warrant a bump?
In any case, will revert. Just curious!
There was a problem hiding this comment.
impl_version is when it is a change in the code without a change in the specification in the runtime.
I guess, both spec_version and impl_version will be bumped automatically using the prdoc information, but I can't confirm
| use super::{BlockNumberFor as NewBlockNumberFor, *}; | ||
| use frame_system::pallet_prelude::BlockNumberFor as LocalBlockNumberFor; | ||
|
|
||
| /// Converts previous (local) block number into the new one. May just be identity functions |
There was a problem hiding this comment.
| /// Converts previous (local) block number into the new one. May just be identity functions | |
| /// Converts old block number into the new one. May just be identity functions |
Is there an inherent local/remote duality, or is it just old/new?
There was a problem hiding this comment.
Inherent local -> whatever you've now decided (new)
In the context of this migration because the salary pallet previously did not allow selection of a block provider, it defaulted to frame_system's, which is always local I believe. However, @gui1117 did point out this code may be copy pasted so if you think generalizing it is best, I can make the change 👍 Just let me know.
| /// | ||
| /// For instance - if your new version uses the relay chain number, you'll want to | ||
| /// use relay current - ((current local - local) * equivalent_block_duration) | ||
| fn equivalent_moment_in_time(local: L) -> N; |
There was a problem hiding this comment.
Can you put some example in the doc of this please? Just some pseudo code of how it is supposed to be called.
I can imagine it is easy to call it the other way around of how its meant to be 🙈
There was a problem hiding this comment.
Have updated the docs for each function. Let me know if sufficient
|
Looks like there's still some issues to fix. Hold off on re-review 🙏 |
Following #3617, salary pallet is to be made async backing friendly. This PR adds the block number provider config parameter to pallet-salary.
In addition it
TODO:
Once Westend is updated I will write the migration for polkadot collectives.
@xlc
@gupnik Tracking list