-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Make Staking pallet using a proper Time module. #4662
Conversation
Any update on this? |
OK I made an alternative implementation, otherwise I can do now_millis but I feel like this way the moment of |
/// Duration for a Julian year (365.25 days). | ||
fn year() -> Self::Moment; | ||
|
||
/// Return current moment. | ||
fn now() -> Self::Moment; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you are mixing some time related functionality (year()
) with a moment
that can be anything.
I still think we should keep this Time
trait simple and say that now()
will return a moment
that increases between blocks, but without specifying what it is. It could also be the moment from the genesis of the chain.
I would add another trait UnixTime
that clearly says that now()
returns the Duration
from the unix epoch. I would directly return core::time::Duration
. This already brings all the functions for converting between the different time formats.
The timestamp module would implement both of these traits and return the same value for both now()
calls, but as different types. Staking
would require that Time
implements UnixTime
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I completely agree
please update to latest master. |
I think it should be explained in the timestamp module when another module (i.e. staking in this case) should use |
Co-Authored-By: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Why? It depends on what the module requires on constraints and not what the timestamp module provides. You could just have the requirement of time increasing which could be anything, even just the block number. Or like staking you really need UNIX time. |
to be honest I struggle to know what are the guarantee of Time, maybe that it can only increase. |
frame/staking/src/lib.rs
Outdated
/// Deprecated Time. | ||
/// | ||
/// Deprecated associated type wrongly used to query Time in millisecond. | ||
type DeprecatedTime: Time; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main use it to be able to decode the old structure ActiveInfo<MomentOf<T>>
.
But indeed I can just decode the EraIndex and not start field as I don't use it.
I updated code as such, (also fixing that StorageVersion wasn't set)
What is your plan with this @thiolliere ? |
} | ||
|
||
impl Default for Releases { | ||
fn default() -> Self { | ||
Releases::V2_0_0 | ||
Releases::V3_0_0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what to think about this default, because if we add staking in a running chain (so without executing GenesisConfig) then we must insert the storage version in the runtime upgrade.
I think a staking module without any storage version must be considered invalid now.
otherwise if a chain introduce staking module without inserting the storage version then the storage version depends on when it has been inserted, and storage version could be V2 or V3 there is no way to know it.
OK as the strategy for now is only to support Kusama I updated the runtime_upgrade code. The PR is now ready for review |
Please merge master. |
Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>
Needed a polkadot companion PR :( |
sorry done here paritytech/polkadot#941 |
(EDITED)
the staking modules make use of Time trait to get the time in millisecond, however this a wrong usage of such traits which doesn't specifies what is moment.
To solve this this PR introduce a new trait named UnixTime implemented by timestamp and used by staking.
As said by Kian it would be great to have more docs on the Time trait itself and usage it is suppose to have. But I struggle to define any documentation for this trait.
The migration is implemented in such a way that the active era start at the moment of the migration.
This is needed because even if the wrong usage of Time correctly returned a moment in millisecond there is no guarantee that this moment is since unix_epoch.
On Kusama however the old Time was indeed the millisecond since unix epoch thus we could improve kusama migration.