docs(adr): propose epoch module #19474
Conversation
WalkthroughThe recent updates involve two significant changes. Firstly, the abandonment of the proposal for epoched staking, marking a shift in development focus or strategy. Secondly, the introduction of a new Epoch Module, aiming to enhance the Cosmos SDK by allowing better coordination and control over module execution timing. This module is designed to improve system performance and block time management by enabling modules to register their logic for execution at predefined intervals. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
This new API seems better, but happy to move the canonical epochs module from Osmosis to the SDK as well if that helps |
it was a quick brain dump so if you have ideas lmk |
|
#19355 will depend on this :) |
There was a problem hiding this comment.
Lookin' good so far! Eager to see how the design plays out. I think it should be pretty simple.
What I'd love to see in the ADR is a high-level overview of design of:
- How to register epoch handlers
- What the handler interface looks like
- Epoch module storing last execution times
Also, I'm not sure if there's a great need to mention BeginBlock and EndBlock much. IMO these two concepts are somewhat orthogonal and mutually beneficial. Perhaps we mention that epochs can build upon these powerful primitives to give modules "cadence" to certain execution handlers.
| RegisterTimeBeginEpochCalls() map[time.seconds]func() error | ||
| RegisterTimeEndEpochCalls() map[time.seconds]func() error |
There was a problem hiding this comment.
With time-based epoch calls, assuming PBTS exist, you'll need the epoch module to store or at least know when the last execution occurred, i.e. x >= time.Now() - LastExecution.
|
marking this as r4r but also happy to upstream the epochs module from osmosis. It seems that has adoption in the cosmos. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
|
||
| ## Abstract | ||
|
|
||
| Currently in the Cosmos SDK all modules have the option to execute logic at the start and end of every block. If a module would like to do something at different intervals, but this logic is not exposed to clients. This makes it confusing when to expect things to be done and not. Secondly, if there are many modules that would like to execute logic at intervals coordinating which modules should run at which block can not be done. This can lead to countless modules running at the same block potentially causing issues with the block time. |
There was a problem hiding this comment.
The sentence structure in this section could be improved for clarity. Consider revising to enhance readability and ensure grammatical correctness. For example, "If a module would like to do something at different intervals, but this logic is not exposed to clients." seems incomplete. Additionally, "cannot" is more commonly used than "can not".
- If a module would like to do something at different intervals, but this logic is not exposed to clients.
+ If a module would like to do something at different intervals, this logic is currently not exposed to clients.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| Currently in the Cosmos SDK all modules have the option to execute logic at the start and end of every block. If a module would like to do something at different intervals, but this logic is not exposed to clients. This makes it confusing when to expect things to be done and not. Secondly, if there are many modules that would like to execute logic at intervals coordinating which modules should run at which block can not be done. This can lead to countless modules running at the same block potentially causing issues with the block time. | |
| Currently in the Cosmos SDK all modules have the option to execute logic at the start and end of every block. If a module would like to do something at different intervals, this logic is currently not exposed to clients. This makes it confusing when to expect things to be done and not. Secondly, if there are many modules that would like to execute logic at intervals coordinating which modules should run at which block can not be done. This can lead to countless modules running at the same block potentially causing issues with the block time. |
|
|
||
| Creating a coordination module that will allow modules to register their logic to be executed at different intervals. This will allow for better coordination of modules and allow for better understanding of when things will be executed. This will also allow for better control of the block time and allow for better performance. | ||
|
|
||
| The module does not handle storage of logic to be executed at a certain time, instead it is meant to be as stateless as possible. The implemention of module logic should not queue logic as well, it should execute but not return the data to respective location. For example, with staking val set updates should happen during the state transition but the returned value to comet for validator set updates only gets returned when the epoch is called. If the epoch is called at every block it will be the same as today. |
There was a problem hiding this comment.
There's a typo and a missing comma in this section. Also, the sentence structure could be improved for clarity. Consider revising to ensure grammatical correctness and clarity.
- The implemention of module logic should not queue logic as well, it should execute but not return the data to respective location.
+ The implementation of module logic should not queue logic as well; it should execute but not return the data to the respective location.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| The module does not handle storage of logic to be executed at a certain time, instead it is meant to be as stateless as possible. The implemention of module logic should not queue logic as well, it should execute but not return the data to respective location. For example, with staking val set updates should happen during the state transition but the returned value to comet for validator set updates only gets returned when the epoch is called. If the epoch is called at every block it will be the same as today. | |
| The module does not handle storage of logic to be executed at a certain time, instead it is meant to be as stateless as possible. The implementation of module logic should not queue logic as well; it should execute but not return the data to the respective location. For example, with staking val set updates should happen during the state transition but the returned value to comet for validator set updates only gets returned when the epoch is called. If the epoch is called at every block it will be the same as today. |
|
|
||
| A new epoch module is created that will allow for modules to register their logic to be executed at different intervals. Modules can register specific calls to be called at specified intervals. The epoch module will be responsible for executing the logic at the specified intervals. | ||
|
|
||
| This changes the current design in which each module that executes logic in begin and/or end block to the epoch module having a map of calls to make each interval. The epoch module will be responsible for calling the logic and/or passing messages to the respective modules. This does not replace the concept of beginblock and endblock but rather allows for modules to register their logic to be executed at different intervals. |
There was a problem hiding this comment.
There are several spelling mistakes and a possible missing comma in this section. Additionally, the use of "begin" seems incorrect in the context provided. Consider revising for accuracy and clarity.
- This changes the current design in which each module that executes logic in begin and/or end block to the epoch module having a map of calls to make each interval.
+ This changes the current design in which each module that executes logic in begin and/or end block, to the epoch module having a map of calls to make at each interval.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| This changes the current design in which each module that executes logic in begin and/or end block to the epoch module having a map of calls to make each interval. The epoch module will be responsible for calling the logic and/or passing messages to the respective modules. This does not replace the concept of beginblock and endblock but rather allows for modules to register their logic to be executed at different intervals. | |
| This changes the current design in which each module that executes logic in begin and/or end block, to the epoch module having a map of calls to make at each interval. The epoch module will be responsible for calling the logic and/or passing messages to the respective modules. This does not replace the concept of beginblock and endblock but rather allows for modules to register their logic to be executed at different intervals. |
|
|
||
| This changes the current design in which each module that executes logic in begin and/or end block to the epoch module having a map of calls to make each interval. The epoch module will be responsible for calling the logic and/or passing messages to the respective modules. This does not replace the concept of beginblock and endblock but rather allows for modules to register their logic to be executed at different intervals. | ||
|
|
||
| This simplifies the need for their to be ordering in begin/endblock because the new design is around a caller to represent ordering. For example in the current design mint must come before distribution in begin block. With the proposed design mint and distribution would not have anything to register in beginblock and instead mint will only register a call to be made at a specific interval. Mint will then call distribution. |
There was a problem hiding this comment.
The use of "their" seems incorrect, and there are several spelling mistakes and missing commas. Consider revising for grammatical correctness and clarity.
- This simplifies the need for their to be ordering in begin/endblock because the new design is around a caller to represent ordering.
+ This simplifies the need for there to be ordering in begin/end block because the new design revolves around a caller to represent ordering.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| This simplifies the need for their to be ordering in begin/endblock because the new design is around a caller to represent ordering. For example in the current design mint must come before distribution in begin block. With the proposed design mint and distribution would not have anything to register in beginblock and instead mint will only register a call to be made at a specific interval. Mint will then call distribution. | |
| This simplifies the need for there to be ordering in begin/end block because the new design revolves around a caller to represent ordering. For example in the current design mint must come before distribution in begin block. With the proposed design mint and distribution would not have anything to register in beginblock and instead mint will only register a call to be made at a specific interval. Mint will then call distribution. |
| } | ||
| ``` | ||
|
|
||
| A module will write a functions that returns an error to execute the specific logic at an interval. In the future with internal message passing this can be done with a message instead of a function. If a module does want to use a message they can do this as well and avoid the need to write the function on the keeper. |
There was a problem hiding this comment.
The sentence structure in this section could be improved for clarity, and there's a grammatical error regarding the use of "a functions". Consider revising to ensure grammatical correctness and clarity.
- A module will write a functions that returns an error to execute the specific logic at an interval.
+ A module will write functions that return an error to execute the specific logic at an interval.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| A module will write a functions that returns an error to execute the specific logic at an interval. In the future with internal message passing this can be done with a message instead of a function. If a module does want to use a message they can do this as well and avoid the need to write the function on the keeper. | |
| A module will write functions that return an error to execute the specific logic at an interval. In the future with internal message passing this can be done with a message instead of a function. If a module does want to use a message they can do this as well and avoid the need to write the function on the keeper. |
|
|
||
| ## Consequences | ||
|
|
||
| * The new modules becomes required for all modules that would allow users to register logic to be executed at different intervals. |
There was a problem hiding this comment.
The verb form 'becomes' does not seem to match the subject 'modules'. Consider revising for grammatical correctness.
- The new modules becomes required for all modules that would allow users to register logic to be executed at different intervals.
+ The new module becomes required for all modules that allow users to register logic to be executed at different intervals.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| * The new modules becomes required for all modules that would allow users to register logic to be executed at different intervals. | |
| * The new module becomes required for all modules that allow users to register logic to be executed at different intervals. |
|
|
||
| ### Negative | ||
|
|
||
| * Modules that require that use the epoch module will need to be modified to avoid using the epoch module, if an application developers does not want to use it. |
There was a problem hiding this comment.
There's a grammatical error regarding the use of "an application developers". Consider revising for grammatical correctness.
- Modules that require that use the epoch module will need to be modified to avoid using the epoch module, if an application developers does not want to use it.
+ Modules that require the use of the epoch module will need to be modified to avoid using the epoch module if an application developer does not want to use it.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| * Modules that require that use the epoch module will need to be modified to avoid using the epoch module, if an application developers does not want to use it. | |
| * Modules that require the use of the epoch module will need to be modified to avoid using the epoch module if an application developer does not want to use it. |
| @@ -11,7 +12,7 @@ | |||
|
|
|||
| ## Status | |||
|
|
|||
| Proposed | |||
| ABANDONED | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The status of ADR 039 as "ABANDONED" is correctly reflected in the ADR document itself. However, the README.md file in the docs/architecture directory does not indicate this status change next to the reference to ADR 039. It is recommended to update the README.md file and any other relevant documentation to clearly reflect the "ABANDONED" status of ADR 039 to ensure clarity and consistency across the documentation.
Analysis chain
The status of this ADR has been updated to "ABANDONED". Ensure that all references to this ADR within the Cosmos SDK documentation and any related proposals are updated accordingly to reflect this change.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to ADR 039 to ensure they reflect the new status.
rg 'ADR 039' docs/
Length of output: 179
Script:
#!/bin/bash
# Check if the README.md file mentions the status "ABANDONED" for ADR 039
rg 'ADR 039' docs/architecture/README.md
# Search for the term "ABANDONED" in relation to ADR 039 across the docs to ensure the status is properly reflected
rg 'ABANDONED' docs/
Length of output: 999
|
upstreaming osmosis version, closing this adr |
Description
This pr proposes a simple epoch module to allow modules to register logic to be run at intervals.
Author Checklist
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.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
Summary by CodeRabbit