[ENG-214] Add new epoch types: year, hour #559
Conversation
- 2 kv stores: identifier => duration, duration => EpochInfo - duration is parsed for numerical ordering of the kv store values - IterateEpochInfo returns epochs in descending numerical order or their durations
Default ordering should be consistent with execution order. Epochs should be executed from smallest to highest. E.g. hourly first, then daily, then weekly, etc. E.g. inflation runs daily. Incentives run weekly. Incentives are given from inflation rewards, so inflation rewards need to be calculated first.
loredanacirstea
force-pushed
the
loredana/ENG-214-add-epoch-types
branch
from
883e89c to
2c6f000
Compare
- add data migration logic - add migration test
loredanacirstea
force-pushed
the
loredana/ENG-214-add-epoch-types
branch
from
2c6f000 to
31335b9
Compare
| @@ -13,6 +13,15 @@ func NewGenesisState(epochs []EpochInfo) *GenesisState { | |||
| // DefaultGenesisState returns the default epochs genesis state | |||
| func DefaultGenesisState() *GenesisState { | |||
| epochs := []EpochInfo{ | |||
| { | |||
| Identifier: YearEpochID, | |||
There was a problem hiding this comment.
@loredanacirstea why do you want to introduce yearly epochs? Considering leap years, the epochs wouldn't be accurate using a Duration of time.Hour * 24 * 365
There was a problem hiding this comment.
I added yearly because it is frequent in business contracts. If you open this API to smart contracts (like I showed in the cron precompile demos https://www.youtube.com/watch?v=1whMSUM0JyQ), you can transcribe the business contract into a smart contract.
If the epochs module cannot have yearly, or monthly (also frequently used), maybe we should think about how it can be rewritten, so it can.
There was a problem hiding this comment.
@loredanacirstea Nice work! Can you reduce this PR to only add hour epochs for now? This PR already holds value because of its improvement on how we store the durations.
As the epoch module is only relevant for other modules on the Evmos protocol right now, we should wait to implement identifiers like month or year until there is a concrete need for another module.
I also suggest to simplify the wording EpochInfo -> Epoch in this PR
| } | ||
|
|
||
| // GetEpochInfo returns epoch info by duration | ||
| func (k Keeper) GetEpoch(ctx sdk.Context, duration []byte) (types.EpochInfo, bool) { |
There was a problem hiding this comment.
Would merge GetEpoch with GetEpochInfo, as right now it's confusing to have both and GetEpoch is only called by GetEpochInfo
There was a problem hiding this comment.
@loredanacirstea what do you think of renaming EpochInfo to Epoch in the proto files? This will make the code less wordy and I don't see the benefit in have Info added.
| } | ||
|
|
||
| // SetEpochInfo set epoch duration by identifier | ||
| func (k Keeper) setEpoch(ctx sdk.Context, epoch types.EpochInfo) { |
There was a problem hiding this comment.
same here Epoch and EpochInfo seem mixed
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days-before-close if no further activity occurs. |
Description
Depends on #553
Add new epoch types: year, hour
Closes: #538
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
PR review checkboxes:
I have...
Unreleasedsection inCHANGELOG.mdCode maintenance:
I have...
godocand code comments.docs/) or specification (x/<module>/spec/)Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.
I have...