BM-13: feat(epoching): add hooks, events and keeper functions #14
Conversation
SebastianElvis
changed the title
|
UPDATE: added 3 keeper functions in this PR to close BM-14 also, given the light workload. |
x/epoching/types/expected_keepers.go
Outdated
| BeginEpoch(ctx sdk.Context, epoch sdk.Uint) error // Must be called when an epoch begins | ||
| EndEpoch(ctx sdk.Context, epoch sdk.Uint) error // Must be called when an epoch ends |
There was a problem hiding this comment.
Just a minor comment about the naming. For example, the statking module has hooks name like AfterValidatorCreated. Should we follow their convention? Maybe AfterEpochBegins?
There was a problem hiding this comment.
Agree. I double-checked Cosmos SDK code, it seems that they distinguish between BeforeXXX andEndXXX. In our scenario I think we only need AfterEpochBegins and AfterEpochEnds. Thanks!
x/epoching/types/expected_keepers.go
Outdated
| BeginEpoch(ctx sdk.Context, epoch sdk.Uint) error // Must be called when an epoch begins | ||
| EndEpoch(ctx sdk.Context, epoch sdk.Uint) error // Must be called when an epoch ends |
There was a problem hiding this comment.
Just a minor comment about the naming. For example, the statking module has hooks name like AfterValidatorCreated. Should we follow their convention? Maybe AfterEpochBegins?
There was a problem hiding this comment.
Looks good (although I don't know much about hooks)!
My two remarks are basically that we already fixed in the proto messages that only certain messages can be delayed, so I reckon the code should reflect that restriction as well, and that I still don't understand why we should support querying delayed messages by epoch number.
x/epoching/keeper/keeper.go
Outdated
| // GetEpochMsgs returns the set of messages queued of a given uncheckpointed epoch | ||
| func (k Keeper) GetEpochMsgs(ctx sdk.Context, epoch sdk.Uint) []sdk.Msg { |
There was a problem hiding this comment.
I thought in https://github.com/babylonchain/babylon/pull/10/files#r899915268 we agreed that this method will not have any epoch parameter, that it returns the messages that will be dequeued at the end of the current epoch. We also know that the return value will be more specific than sdk.Msg, it's the QueuedMessage type you defined.
There was a problem hiding this comment.
Yeah I was doing this for being consistent. Will remove the epoch field and use the QueuedMessage type.
x/epoching/keeper/keeper.go
Outdated
| } | ||
|
|
||
| // EnqueueMsg enqueues a message to the queue of the current epoch | ||
| func (k Keeper) EnqueueMsg(ctx sdk.Context, msg sdk.Msg) error { |
There was a problem hiding this comment.
Shouldn't msg be of type QueuedMessage?
|
Thanks for the comments Akosh and Gai! Now this PR reflects to the comments and #10. Feel free to have a look again. |
Fixes BM-13 and BM-14
This PR adds hooks and events for the epoching module, including
AfterEpochBeginsandAfterEpochEndsBeginEpochandEndEpoch