Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(x/epochs): upstream osmosis epoch module #19697

Merged
merged 48 commits into from
Apr 4, 2024

Conversation

hieuvubk
Copy link
Contributor

@hieuvubk hieuvubk commented Mar 8, 2024

Description

Closes: #19688


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...

  • Scaffold Epochs module
  • Passed unit tests
  • Added autocli
  • Added to simapp

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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Introduced the epochs module with functionalities such as fast reflection for Module struct, gRPC client and server interfaces for querying epoch information, and managing epochs within the Cosmos SDK module.
    • Added new dependency update configuration for gomod package ecosystem.
    • Implemented new job test-x-epochs for running tests and SonarCloud analysis.
    • Added AutoCLI options for querying epoch information.
    • Provided a structured template for documenting changes in versions across different components.
    • Introduced functionality for handling epoch transitions, updating epoch information, and managing epoch hooks.
  • Bug Fixes

    • Corrected import ordering in x/accounts/defaults/lockup/types/encoding.go.
  • Documentation

    • Updated README.md to describe the functionality of the epochs module.
    • Added comments and summaries in various files for better understanding of the code changes.
  • Refactor

    • Reorganized import statements and declarations in multiple files for clarity.
  • Tests

    • Added numerous test cases across the module to validate epoch management functionalities, including epoch incrementation, exporting and initializing epochs, and handling hooks.

Copy link
Contributor

coderabbitai bot commented Mar 8, 2024

Important

Auto Review Skipped

Review was skipped due to path filters

Files ignored due to path filters (1)
  • tests/go.mod is excluded by !**/*.mod

Walkthrough

The update integrates the Epochs module from Osmosis into the Cosmos SDK, enhancing epoch management capabilities. Configuration adjustments, module integration, and feature enhancements across various files aim to streamline epoch tracking, improve gRPC query functionality, and enhance error handling and logging mechanisms within the Cosmos network.

Changes

Files Change Summary
.github/dependabot.yml, .github/pr_labeler.yml Added configurations for dependency updates and PR labeling.
.github/workflows/test.yml Introduced a new job for testing and SonarCloud analysis.
api/cosmos/epochs/..., x/epochs/proto/... Added gRPC interfaces, messages, and configuration for epoch queries.
baseapp/cache_ctx.go, x/epochs/types/cache_ctx.go Introduced functions for error handling, logging, and out-of-gas error checks.
x/epochs/keeper/..., x/epochs/types/... Added logic for managing epochs, including hooks, state initialization, and transitions.
simapp/app.go, simapp/go.mod Defined types, messages, and module dependencies for the epochs functionality in the app.
testutil/list.go, x/epochs/... Updated tests and utilities for sorting and epoch management.

Assessment against linked issues

Objective Addressed Explanation
Upstream Epochs module from Osmosis to SDK [#19688]

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

x/epochs/keeper/epoch.go Fixed Show fixed Hide fixed
x/epochs/keeper/epoch.go Fixed Show fixed Hide fixed
x/epochs/keeper/abci.go Fixed Show fixed Hide fixed
x/epochs/keeper/abci.go Fixed Show fixed Hide fixed
x/epochs/keeper/abci.go Fixed Show fixed Hide fixed
@julienrbrt julienrbrt self-assigned this Mar 8, 2024
baseapp/cache_ctx.go Fixed Show fixed Hide fixed
@tac0turtle tac0turtle changed the title Decentrio/upstream epochs feat(x/epochs): upstream osmosis epoch module Mar 8, 2024
@tac0turtle tac0turtle self-assigned this Mar 8, 2024
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add a go.mod for epochs as well

@hieuvubk
Copy link
Contributor Author

hieuvubk commented Mar 8, 2024

we should add a go.mod for epochs as well

I think its already added

@hieuvubk hieuvubk marked this pull request as ready for review March 12, 2024 06:44
@hieuvubk hieuvubk requested a review from a team as a code owner March 12, 2024 06:44
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 9

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 0a98bfb and be9f85f.
Files selected for processing (51)
  • .github/dependabot.yml (1 hunks)
  • .github/pr_labeler.yml (1 hunks)
  • .github/workflows/test.yml (1 hunks)
  • api/cosmos/epochs/module/v1/module.pulsar.go (1 hunks)
  • api/cosmos/epochs/v1beta1/events.pulsar.go (1 hunks)
  • api/cosmos/epochs/v1beta1/genesis.pulsar.go (1 hunks)
  • api/cosmos/epochs/v1beta1/query.pulsar.go (1 hunks)
  • api/cosmos/epochs/v1beta1/query_grpc.pb.go (1 hunks)
  • baseapp/cache_ctx.go (1 hunks)
  • go.work.example (1 hunks)
  • simapp/app.go (8 hunks)
  • simapp/go.mod (2 hunks)
  • testutil/list.go (2 hunks)
  • x/epochs/CHANGELOG.md (1 hunks)
  • x/epochs/README.md (1 hunks)
  • x/epochs/autocli.go (1 hunks)
  • x/epochs/depinject.go (1 hunks)
  • x/epochs/go.mod (1 hunks)
  • x/epochs/go.sum (1 hunks)
  • x/epochs/keeper/abci.go (1 hunks)
  • x/epochs/keeper/abci_test.go (1 hunks)
  • x/epochs/keeper/epoch.go (1 hunks)
  • x/epochs/keeper/epoch_test.go (1 hunks)
  • x/epochs/keeper/genesis.go (1 hunks)
  • x/epochs/keeper/genesis_test.go (1 hunks)
  • x/epochs/keeper/grpc_query.go (1 hunks)
  • x/epochs/keeper/grpc_query_test.go (1 hunks)
  • x/epochs/keeper/hooks.go (1 hunks)
  • x/epochs/keeper/keeper.go (1 hunks)
  • x/epochs/keeper/keeper_test.go (1 hunks)
  • x/epochs/module.go (1 hunks)
  • x/epochs/proto/buf.gen.gogo.yaml (1 hunks)
  • x/epochs/proto/buf.gen.pulsar.yaml (1 hunks)
  • x/epochs/proto/buf.lock (1 hunks)
  • x/epochs/proto/buf.yaml (1 hunks)
  • x/epochs/proto/cosmos/epochs/module/v1/module.proto (1 hunks)
  • x/epochs/proto/cosmos/epochs/v1beta1/events.proto (1 hunks)
  • x/epochs/proto/cosmos/epochs/v1beta1/genesis.proto (1 hunks)
  • x/epochs/proto/cosmos/epochs/v1beta1/query.proto (1 hunks)
  • x/epochs/simulation/genesis.go (1 hunks)
  • x/epochs/sonar-project.properties (1 hunks)
  • x/epochs/types/events.pb.go (1 hunks)
  • x/epochs/types/genesis.go (1 hunks)
  • x/epochs/types/genesis.pb.go (1 hunks)
  • x/epochs/types/hooks.go (1 hunks)
  • x/epochs/types/hooks_test.go (1 hunks)
  • x/epochs/types/identifier.go (1 hunks)
  • x/epochs/types/keys.go (1 hunks)
  • x/epochs/types/query.pb.go (1 hunks)
  • x/epochs/types/query.pb.gw.go (1 hunks)
  • x/epochs/types/telemetry.go (1 hunks)
Files not summarized due to errors (4)
  • api/cosmos/epochs/v1beta1/events.pulsar.go: Error: Message exceeds token limit
  • api/cosmos/epochs/v1beta1/genesis.pulsar.go: Error: Message exceeds token limit
  • api/cosmos/epochs/v1beta1/query.pulsar.go: Error: Message exceeds token limit
  • x/epochs/go.sum: Error: Message exceeds token limit
Files not reviewed due to errors (6)
  • (no review received)
  • (no review received)
  • (no review received)
  • (no review received)
  • (no review received)
  • (no review received)
Files skipped from review due to trivial changes (5)
  • .github/pr_labeler.yml
  • go.work.example
  • x/epochs/CHANGELOG.md
  • x/epochs/proto/buf.lock
  • x/epochs/types/keys.go
Additional comments: 108
x/epochs/proto/buf.gen.gogo.yaml (1)
  • 1-8: The configuration for protobuf generation using gocosmos and grpc-gateway plugins looks correct and aligns with standard practices for Cosmos SDK modules. This setup enables gRPC and RESTful JSON APIs for the epochs module.
x/epochs/proto/cosmos/epochs/module/v1/module.proto (1)
  • 1-12: The definition of the Module message and the specification of the go_import option are correctly set up for integrating the epochs module with the Cosmos SDK's module system.
x/epochs/sonar-project.properties (1)
  • 1-14: The configuration for SonarCloud properties, including project key, organization, name, and settings for source and test files, looks correct and aligns with standard practices for SonarCloud integration.
x/epochs/proto/buf.yaml (1)
  • 1-18: The configuration for the buf tool, including dependencies, lint rules, and exceptions, looks correct and aligns with standard practices for protobuf linting and dependency management.
x/epochs/proto/buf.gen.pulsar.yaml (1)
  • 1-18: The configuration for protobuf generation with Pulsar and gRPC support looks correct, including the use of paths=source_relative option, which is standard for Go modules.
testutil/list.go (1)
  • 14-24: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3-24]

The addition of the SortSlice function and new imports, including constraints and sort, enhances the testutil package's functionality. The use of math/rand is appropriate in this context, as it's used for test utilities, not for cryptographic purposes.

x/epochs/types/identifier.go (1)
  • 1-24: The functions ValidateEpochIdentifierInterface and ValidateEpochIdentifierString are correctly implemented for validating epoch identifiers. The error messages are clear and informative, ensuring easy debugging and validation.
x/epochs/proto/cosmos/epochs/v1beta1/events.proto (1)
  • 1-22: The event message definitions EventEpochEnd and EventEpochStart are correctly implemented, with appropriate use of int64 for epoch number and start time. These definitions enable clear event signaling for epoch transitions.
x/epochs/keeper/genesis.go (1)
  • 1-24: The InitGenesis and ExportGenesis functions are correctly implemented for initializing and exporting genesis state for the epochs module. The error handling in InitGenesis and the use of DefaultGenesis in ExportGenesis are appropriate for their respective purposes.
x/epochs/keeper/grpc_query_test.go (1)
  • 1-22: The test function TestQueryEpochInfos is correctly implemented, effectively verifying the functionality of the epochs module's gRPC query client. The setup and assertions are appropriate for checking the default genesis epoch infos against expected values.
x/epochs/keeper/hooks.go (2)
  • 7-11: The implementation of AfterEpochEnd correctly follows the intended design of not handling errors explicitly, as mentioned in the comment. This approach relies on the assumption that osmoutils.ApplyFuncIfNoError() effectively manages error scenarios. Ensure that this utility function is robust and well-tested to prevent any unhandled errors from causing unexpected behavior.
  • 13-17: Similarly, BeforeEpochStart also omits explicit error handling, relying on the same utility function as AfterEpochEnd. It's crucial to verify the reliability of osmoutils.ApplyFuncIfNoError() in managing errors, as any oversight could lead to silent failures or bugs in the epochs module's lifecycle events.
x/epochs/types/telemetry.go (1)
  • 3-12: The definition of EpochHookFailedMetricName is well-structured, providing clear labels to categorize the nature of the hook failure. This metric will be valuable for monitoring and debugging purposes. Ensure that the metric is correctly integrated with the telemetry system and that labels are consistently applied across different modules to maintain uniformity in monitoring.
x/epochs/autocli.go (1)
  • 8-27: The AutoCLIOptions function correctly defines CLI options for querying epoch information, enhancing the usability of the epochs module. The inclusion of EpochInfos and CurrentEpoch RPC methods with clear descriptions and usage instructions is commendable. Ensure that these CLI options are well-documented in the module's user guide to facilitate easy access and understanding for end-users.
x/epochs/depinject.go (1)
  • 18-22: The registration of the epochs module with the application configuration using appconfig.RegisterModule is correctly implemented. This setup ensures that the module's dependencies are properly managed and injected. It's important to verify that all required dependencies for the epochs module are included in ModuleInputs and that ModuleOutputs correctly reflects the components provided by the module.
x/epochs/proto/cosmos/epochs/v1beta1/query.proto (2)
  • 11-21: The gRPC querier service definition for the epochs module is well-structured, providing clear and concise RPC methods for querying epoch information. The use of HTTP annotations for RESTful access is a good practice, enhancing the module's accessibility. Ensure that the gRPC service is correctly registered and that the endpoints are tested to validate their functionality.
  • 23-33: The request and response message types for EpochInfos and CurrentEpoch are correctly defined, facilitating clear communication between clients and the epochs module. It's important to ensure that these message types are used consistently across the module and that their fields are adequately documented to aid in understanding and usage.
x/epochs/simulation/genesis.go (3)
  • 14-17: The GenDuration function correctly generates a random duration for epoch configuration in simulations. However, the use of math/rand is flagged by static analysis for security reasons. In the context of simulations, using math/rand is generally acceptable for performance reasons and because cryptographic security is not a requirement. Ensure that this choice is documented to clarify the rationale.
  • 19-30: The RandomizedEpochs function effectively generates a random list of epoch configurations for simulation purposes. This function enhances the robustness of simulations by testing various epoch configurations. Ensure that the generated configurations are diverse enough to cover a wide range of scenarios and that the simulations are thoroughly tested.
  • 32-46: The RandomizedGenState function correctly generates a random genesis state for the epochs module, incorporating the randomly generated epoch configurations. This approach is valuable for testing the module's behavior under different initial conditions. Ensure that the generated genesis state is validated to prevent invalid configurations from being used in simulations.
x/epochs/keeper/keeper.go (3)
  • 24-39: The NewKeeper constructor is correctly implemented, setting up the Keeper struct with necessary dependencies and initializing the EpochInfo map. The use of collections.NewSchemaBuilder and collections.NewMap enhances modularity and type safety. Ensure that error handling for sb.Build() is robust and that any initialization errors are appropriately managed to prevent runtime issues.
  • 41-50: The SetHooks method provides a mechanism for setting epoch hooks, enforcing that hooks can only be set once. This design prevents accidental overwrites and ensures that the hooks are consistently applied. Consider adding documentation or examples to illustrate how to correctly implement and use epoch hooks within the module.
  • 52-54: The Logger method correctly provides access to the module's logger, facilitating logging throughout the epochs module. Ensure that logging is used judiciously to provide valuable insights into the module's operations without overwhelming the log output.
x/epochs/keeper/grpc_query.go (3)
  • 22-25: The NewQuerier function correctly initializes a new Querier instance, encapsulating the epochs module's keeper. This design pattern enhances modularity and separates concerns between the keeper's state management and the gRPC query interface. Ensure that the Querier is correctly registered with the gRPC server to enable query functionality.
  • 27-34: The EpochInfos method provides a gRPC query interface for retrieving running epoch information. The implementation correctly unwraps the SDK context and utilizes the keeper's AllEpochInfos method. Ensure that this method is thoroughly tested, including scenarios with no epochs configured, to validate its behavior under various conditions.
  • 36-55: The CurrentEpoch method correctly validates the request parameters, ensuring that the request is not nil and that the identifier is not empty. This validation is crucial for preventing errors and ensuring meaningful responses. The method then retrieves and returns the current epoch information for the specified identifier. Ensure that error messages are clear and informative to aid in debugging and user interaction.
x/epochs/types/genesis.go (3)
  • 11-23: The NewGenesisState and DefaultGenesis functions correctly initialize the genesis state for the epochs module, providing a mechanism for setting up default epoch configurations. The choice of default epochs (day, hour, week) is sensible and provides a good starting point for module configuration. Ensure that the default configurations are well-documented to aid users in understanding and customizing the genesis state.
  • 25-39: The Validate method for GenesisState correctly checks for unique epoch identifiers and delegates to the Validate method of each EpochInfo instance. This comprehensive validation ensures the integrity of the genesis state. Consider adding more detailed error messages, including the problematic identifier, to facilitate debugging and configuration adjustments.
  • 41-56: The Validate method for EpochInfo correctly checks for non-empty identifiers, non-zero durations, and non-negative epoch numbers and start heights. These validations are crucial for ensuring the correctness of epoch configurations. Ensure that these validation rules are clearly documented in the module's user guide to assist users in configuring epochs correctly.
x/epochs/keeper/abci.go (1)
  • 13-64: The BeginBlocker method is well-implemented, providing a comprehensive mechanism for managing epoch transitions based on block time and height. The method correctly determines when a new epoch should start and handles initial epoch starts and subsequent epoch transitions. Emitting events for epoch start and end enhances observability and integration with other modules. Ensure that the logic for determining epoch transitions is thoroughly tested, including edge cases around the start and end times.
api/cosmos/epochs/v1beta1/query_grpc.pb.go (13)
  • 7-7: The package name epochsv1beta1 is appropriately named following Go conventions for versioned packages.
  • 9-14: The import statements are correctly organized and include necessary packages for the gRPC service.
  • 21-24: Constants for full method names are correctly defined, ensuring easy reference and maintenance.
  • 26-34: The QueryClient interface is well-defined, providing clear documentation and method signatures for interacting with the epochs service.
  • 36-42: The queryClient struct and its constructor NewQueryClient are correctly implemented, following best practices for structuring client interfaces in gRPC services.
  • 44-60: The client methods EpochInfos and CurrentEpoch are correctly implemented, with proper error handling and response struct initialization.
  • 62-71: The QueryServer interface is correctly defined, including all necessary methods and the mustEmbedUnimplementedQueryServer for forward compatibility.
  • 73-83: The UnimplementedQueryServer struct provides stubs for the server methods, correctly implementing the interface and ensuring forward compatibility.
  • 85-90: The UnsafeQueryServer interface is correctly defined as a means to opt-out of forward compatibility, with a clear warning about its use.
  • 92-94: The RegisterQueryServer function is correctly implemented, allowing for the registration of the Query service with a gRPC server.
  • 96-112: The handler function _Query_EpochInfos_Handler is correctly implemented, ensuring proper request decoding, method invocation, and interceptor handling.
  • 114-130: The handler function _Query_CurrentEpoch_Handler follows the same correct implementation pattern as _Query_EpochInfos_Handler, ensuring consistency and correctness.
  • 132-150: The Query_ServiceDesc variable is correctly defined, providing a complete description of the Query service for gRPC server registration.
x/epochs/README.md (23)
  • 23-23: Consider adding an apostrophe to "epochs" to indicate possession or contraction, depending on the intended meaning.
  • 29-29: The term "mainnet" is correctly spelled as one word in the context of blockchain technology.
  • 38-38: The term "Epochs module" does not require an apostrophe in this context.
  • 38-38: The term "EpochInfo" is correctly used as a single word in the context of code or technical documentation.
  • 40-40: The term "EpochInfos" is correctly used as a single word in the context of code or technical documentation.
  • 48-48: The term "BeginBlocker" is correctly used as one word in the context of blockchain technology.
  • 50-50: Consider removing the extra whitespace for consistency in formatting.
  • 52-52: Consider removing the extra whitespace for consistency in formatting.
  • 53-53: Consider removing the extra whitespace for consistency in formatting.
  • 55-55: The term "EndBlocker" is correctly used as one word in the context of blockchain technology.
  • 57-57: Consider removing the extra whitespace for consistency in formatting.
  • 59-59: Consider removing the extra whitespace for consistency in formatting.
  • 65-65: Remove the space before the closing parenthesis for consistency in code block formatting.
  • 83-83: Add a space between sentences for better readability.
  • 94-94: The term "epochIdentifier" is correctly used as one word in the context of code or technical documentation.
  • 95-95: The term "epochIdentifier" is correctly used as one word in the context of code or technical documentation.
  • 96-96: The term "epochIdentifier" is correctly used as one word in the context of code or technical documentation, and "Params" is correctly capitalized as it refers to a specific code entity.
  • 99-99: Remove the space after the opening parenthesis for consistency in code block formatting.
  • 124-124: Remove the space before the closing parenthesis for consistency in code block formatting.
  • 135-135: The term "epochInfos" is correctly used as one word in the context of code or technical documentation.
  • 137-137: The term "epochInfos" is correctly used as one word in the context of code or technical documentation.
  • 169-169: The term "osmosisd" is correctly used in the context of a command-line example.
  • 177-177: The term "osmosisd" is correctly used in the context of a command-line example.
.github/dependabot.yml (1)
  • 245-253: The configuration for automated dependency updates for the /x/epochs directory has been correctly added, specifying weekly updates on Wednesdays at 03:15 with automerge labels. This setup will help keep the dependencies for the epochs module up to date.
x/epochs/go.mod (4)
  • 1-1: The module path cosmossdk.io/x/epochs is correctly specified, ensuring that the epochs module can be properly imported and used within the Cosmos SDK ecosystem.
  • 3-3: The Go version is set to 1.21, which is appropriate for ensuring compatibility with the latest features and security updates of the Go programming language.
  • 5-158: The dependencies listed in the go.mod file are comprehensive and include both direct and indirect dependencies necessary for the epochs module. It's important to periodically review and update these dependencies to maintain compatibility and security.
  • 160-168: The additional indirect dependencies specified in the go.mod file support various functionalities and integrations within the epochs module. Keeping these dependencies up to date is crucial for the module's performance and security.
x/epochs/keeper/abci_test.go (1)
  • 17-104: The test cases in TestEpochInfoBeginBlockChanges are well-structured and cover various scenarios for epoch changes at block beginnings. It's good practice to include comments explaining the purpose of each test case, which is done here. However, consider adding more detailed comments within each test case to explain the logic behind specific assertions, especially for complex scenarios. This will improve maintainability and readability for future contributors.
x/epochs/types/query.pb.gw.go (1)
  • 1-236: The gRPC gateway setup in query.pb.gw.go correctly translates gRPC calls into RESTful JSON APIs for the epochs module. The use of runtime.Marshaler for handling request and response marshaling is appropriate. Ensure that all gRPC endpoints are properly secured and authenticated, especially if they are exposed publicly. Consider documenting the RESTful endpoints generated by this file in the module's README or API documentation for better developer experience.
simapp/go.mod (1)
  • 43-43: The addition of cosmossdk.io/x/epochs to the require section with a pseudo-version (v0.0.0-00010101000000-000000000000) is noted. Ensure that this pseudo-version is intentional and temporary until a stable version of the epochs module is released. It's crucial to update this dependency to a stable version as soon as it becomes available to avoid potential issues with module compatibility and stability.
x/epochs/types/events.pb.go (4)
  • 26-29: The addition of the EventEpochEnd struct correctly follows protobuf-generated Go file conventions. The struct is well-defined with a single field EpochNumber, which is appropriately tagged for protobuf serialization. This change aligns with the PR's objectives of integrating the epochs module, as it provides a way to emit events at the end of an epoch.
  • 71-75: The EventEpochStart struct is similarly well-defined and follows the conventions for protobuf-generated Go files. It includes two fields: EpochNumber and EpochStartTime, both of which are essential for signaling the start of an epoch and providing relevant data. This struct supports the PR's goal of enhancing epoch management within the Cosmos SDK.
  • 31-60: The methods associated with EventEpochEnd are correctly generated and follow the expected patterns for protobuf-generated Go code. These methods ensure that instances of EventEpochEnd can be serialized, deserialized, and utilized effectively within the Cosmos SDK. It's important that these methods are not manually altered, as they are part of the autogenerated code.
  • 77-106: Similarly, the methods associated with EventEpochStart are correctly generated and adhere to the expected patterns for protobuf-generated Go code. These methods enable the serialization, deserialization, and general use of EventEpochStart instances within the Cosmos SDK. As with EventEpochEnd, any manual alterations to these methods should be avoided to maintain the integrity of the autogenerated code.
api/cosmos/epochs/module/v1/module.pulsar.go (6)
  • 1-1: The file header correctly indicates that this is a generated file and should not be manually edited. This is a good practice as it informs developers not to make changes directly to this file.
  • 4-14: The import section is well-organized and includes necessary packages for the protobuf message functionality. The use of an underscore import for "cosmossdk.io/api/cosmos/app/v1alpha1" implies that the package is imported for its side-effects only, which is common in Go for certain initialization tasks. Ensure that this side-effect import is indeed necessary for the module's functionality.
  • 20-23: Initialization function init() correctly initializes the protobuf message descriptor for Module. This is standard practice for protobuf-generated Go files.
  • 385-390: The definition of the Module struct includes protoimpl.MessageState, protoimpl.SizeCache, and protoimpl.UnknownFields. These fields are used internally by the protobuf implementation to manage message state, caching, and unknown fields encountered during parsing, respectively. This is a standard pattern in protobuf-generated Go code.
  • 392-399: The Reset method correctly resets the Module instance to its zero value and re-initializes message info if protoimpl.UnsafeEnabled is true. This method is crucial for reusing protobuf message instances.
  • 412-502: The file descriptor and various initialization functions (file_cosmos_epochs_module_v1_module_proto_init, etc.) are correctly defined to set up the protobuf message's metadata and ensure that the file is only initialized once. This setup is crucial for the protobuf runtime to correctly handle the Module message.
x/epochs/types/genesis.pb.go (5)
  • 4-17: The package and import statements are correctly structured. The use of blank imports (_) for certain packages like github.com/cosmos/gogoproto/gogoproto, google.golang.org/protobuf/types/known/durationpb, and google.golang.org/protobuf/types/known/timestamppb is appropriate for ensuring the necessary side effects, such as type registration or interface compliance, are achieved. This is a common pattern in Go, especially with generated code.
  • 31-74: The EpochInfo struct is well-defined and documented, providing clear insights into its purpose and each field's role within the epochs module. The use of protobuf field tags alongside JSON and YAML tags ensures compatibility and ease of use across different serialization formats. The application of stdtime and stdduration options for time.Time and time.Duration fields, respectively, is a good practice, leveraging gogoproto's capabilities for more natural Go types in protobuf-generated code.
  • 158-161: The GenesisState struct is succinct and effectively captures the initial state required for the epochs module at genesis. It holds a slice of EpochInfo, allowing for the definition of multiple epochs from the outset. The struct is appropriately annotated for protobuf serialization, ensuring compatibility and ease of use.
  • 25-29: The compile-time assertion to ensure compatibility with the proto package version is a good practice. It helps catch issues early if the proto package version used is not compatible with the generated code expectations. This kind of assertion is common in protobuf-generated code and helps maintain code reliability.
  • 76-417: The utility functions and marshaling/unmarshaling methods are correctly implemented, following the standard patterns expected in protobuf-generated Go code. These methods ensure that instances of EpochInfo and GenesisState can be efficiently serialized and deserialized, interacted with using standard Go idioms (like the String method for debugging), and reset to their zero values when needed. The use of gogoproto types for time-related fields and the corresponding custom marshaling methods enhance compatibility and ease of use.
x/epochs/types/query.pb.go (6)
  • 6-20: While reviewing the imports, it's noted that there are several underscore imports (_ "github.com/cosmos/cosmos-sdk/types/query", _ "github.com/cosmos/gogoproto/gogoproto", _ "google.golang.org/genproto/googleapis/api/annotations"). These are typically used to ensure the initialization functions of these packages are called, which is common in Go for packages that have side effects. However, it's important to ensure that these imports are indeed necessary for this file, as unnecessary imports can lead to bloated binaries and potentially slower compile times.

Please verify that each underscore import is necessary for the proper functioning of the epochs module's query functionality.

  • 33-65: The QueryEpochsInfoRequest struct is defined without any fields. This pattern is common for request types that do not require any input parameters. However, it's crucial to ensure that this design choice aligns with the intended functionality of the EpochInfos RPC method. If future requirements necessitate input parameters for filtering or pagination, this design might need to be revisited.

The current implementation is acceptable given the context, but keep potential future requirements in mind.

  • 69-111: The QueryEpochsInfoResponse struct contains a slice of EpochInfo as its only field. This design is appropriate for returning multiple epoch information records in response to a query. However, it's important to ensure that the EpochInfo type is properly designed to encapsulate all necessary information about an epoch. Additionally, consider the implications of large datasets on the response size and the potential need for pagination.

Consider future-proofing the API by adding pagination support if the number of epochs can grow significantly.

  • 113-155: The QueryCurrentEpochRequest struct includes an Identifier field, which is a string. This design allows clients to query the current epoch by specifying an identifier, which is flexible and accommodates different epoch types or identifiers. Ensure that the identifier is validated properly on the server side to prevent any potential misuse or errors.

The use of an identifier for querying the current epoch is a sound design choice. Ensure proper validation on the server side.

  • 157-198: The QueryCurrentEpochResponse struct includes a CurrentEpoch field of type int64. This is a straightforward and effective way to communicate the current epoch number. However, consider the potential for negative values and ensure that the epoch numbering scheme and validation logic prevent any misuse or misunderstanding of this field.

The response structure for querying the current epoch is clear and effective. Pay attention to validation and documentation to prevent misuse.

  • 248-357: The QueryClient and QueryServer interfaces define the gRPC client and server API for the epochs module. These interfaces are well-structured and follow gRPC best practices. However, ensure that the documentation for each method is clear and provides enough information for developers to understand the purpose and usage of each RPC call.

Enhance the documentation for the EpochInfos and CurrentEpoch methods to include more details about their functionality, expected inputs, and outputs.

simapp/app.go (9)
  • 84-86: The imports for the epochs module have been correctly added. This is necessary for utilizing the epochs module within the SimApp struct and throughout the application.
  • 172-172: The addition of EpochsKeeper to the SimApp struct is crucial for managing epoch-related data and functionality within the application. This change aligns with the PR's objective of integrating the epochs module.
  • 265-265: Adding epochstypes.StoreKey to the NewSimApp function parameters is essential for initializing the epochs module's storage. This ensures that the module's data is correctly persisted and accessible.
  • 410-413: The initialization of EpochsKeeper within the NewSimApp function is correctly implemented. This step is necessary for setting up the epochs module with its required dependencies and configurations.
  • 415-419: Setting hooks for EpochsKeeper is a critical step in integrating the epochs module. This allows for the execution of custom logic at the start and end of epochs, enhancing the module's integration with the broader Cosmos SDK ecosystem. However, it's noted that the actual hook receivers are commented as placeholders. Ensure that specific hook receivers are implemented as needed for the application's requirements.
Verification successful

Based on the provided output, the SetHooks method is indeed utilized across various modules within the codebase, including the EpochsKeeper, StakingKeeper, and GovKeeper in simapp/app.go. The usage of multi-hooks patterns (NewMultiEpochHooks, NewMultiGovHooks, and NewMultiStakingHooks) suggests that specific hook receivers are implemented and integrated into these modules. Although the output does not explicitly detail the hook receivers for EpochsKeeper, the pattern of implementation across modules supports the inference that specific hook receivers are likely in place, aligning with the application's requirements as mentioned in the review comment.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify if epoch hooks receivers are implemented in other parts of the codebase.
rg "SetHooks" --type go

Length of output: 1709

* 447-447: Registering the epochs module in the `ModuleManager` is a necessary step for including it in the application's lifecycle. This ensures that the epochs module can participate in the application's initialization, begin-block, and end-block processes. * 468-468: Adding `epochstypes.ModuleName` to the module order in `NewSimApp` for begin-blockers is correct. This ensures that the epochs module's logic is executed at the appropriate time during the block processing cycle. * 488-488: Including `epochstypes.ModuleName` in the genesis module order is appropriate. This ensures that the epochs module's state is correctly initialized or exported during the genesis process. * 705-707: The addition of printing statements for modules and module names in `AutoCliOpts` is a helpful debugging tool. However, consider if this is intended for production use or if it should be conditional based on a debug or verbose flag to avoid cluttering the application's output in a production environment.

Consider wrapping these print statements with a conditional check for a debug or verbose flag to control their output based on the environment.

.github/workflows/test.yml (1)
  • 1226-1255: The addition of the test-x-epochs job to the GitHub Actions workflow is well-structured and follows the established patterns of other test jobs in the file. It's good to see the use of caching and conditional execution based on Git diffs and PR draft status. However, ensure that the specified Go version (1.22) aligns with the project's current requirements and that the cache dependency path is necessary and correctly specified for the x/epochs module.
api/cosmos/epochs/v1beta1/genesis.pulsar.go (2)
  • 1332-1448: The EpochInfo message structure is well-defined and uses appropriate protobuf types for timestamps and durations. However, it would be beneficial to add more detailed comments explaining the purpose and usage of each field within the context of the epochs module, especially for fields like current_epoch_start_time and current_epoch_start_height, to provide clearer guidance for developers.
  • 1450-1484: The GenesisState message structure is correctly defined, using a repeated field for Epochs to initialize multiple epochs at the genesis of the blockchain. This structure is appropriate and follows protobuf conventions for defining lists of messages.
x/epochs/go.sum (1)
  • 1-1009: Consider verifying if the project can use consistent module versions for dependencies listed multiple times with different versions (e.g., golang.org/x/sys, google.golang.org/grpc). This can help simplify dependency management and reduce potential conflicts.

@@ -247,6 +248,7 @@ replace (
cosmossdk.io/x/bank => ../x/bank
cosmossdk.io/x/circuit => ../x/circuit
cosmossdk.io/x/distribution => ../x/distribution
cosmossdk.io/x/epochs => ../x/epochs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The replace directive for cosmossdk.io/x/epochs pointing to ../x/epochs is appropriate for local development and testing against the latest changes in the epochs module. However, ensure that this directive is removed or updated before releasing the simapp to avoid module resolution issues for end-users who do not have the same local directory structure.

Comment on lines 15 to 23
// FormatMetricName helper to format a metric name given SDK module name and extension.
func FormatMetricName(moduleName, extension string) string {
return moduleName + "_" + extension
}

// formatTxFeesMetricName formats the epochs module metric name.
func formatEpochMetricName(metricName string) string {
return FormatMetricName(ModuleName, metricName)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The helper functions FormatMetricName and formatEpochMetricName are correctly implemented, providing a standardized way to format metric names. This approach enhances consistency and readability in metric naming conventions across the epochs module. Consider adding unit tests for these helper functions to ensure their behavior remains correct across future changes.

Would you like me to generate unit tests for these helper functions?

Comment on lines +40 to +44
func ProvideModule(in ModuleInputs) ModuleOutputs {
k := keeper.NewKeeper(in.Environment, in.Cdc)
m := NewAppModule(in.Cdc, k)
return ModuleOutputs{EpochKeeper: k, Module: m}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ProvideModule function correctly constructs the Keeper and AppModule instances, encapsulating the dependency injection logic for the epochs module. This approach enhances modularity and testability. Ensure that the NewKeeper and NewAppModule functions are thoroughly tested to validate their behavior with various dependency configurations.

Would you like me to generate tests for the NewKeeper and NewAppModule functions?

Every epoch will have a start time, and an end time, where `end time = start time + timer interval`.
On Osmosis mainnet, we only utilize one identifier, with a time interval of `one day`.

The timer will tick at the first block whose blocktime is greater than the timer end time,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The term "blocktime" should be two separate words: "block time".

- the first block whose blocktime is greater than the timer end time,
+ the first block whose block time is greater than the timer end time,

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.

Suggested change
The timer will tick at the first block whose blocktime is greater than the timer end time,
The timer will tick at the first block whose block time is greater than the timer end time,

On Osmosis mainnet, we only utilize one identifier, with a time interval of `one day`.

The timer will tick at the first block whose blocktime is greater than the timer end time,
and set the start as the prior timer end time. (Notably, its not set to the block time!)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace "its" with "it's" to correct the contraction for "it is".

- (Notably, its not set to the block time!)
+ (Notably, it's not set to the block time!)

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.

Suggested change
and set the start as the prior timer end time. (Notably, its not set to the block time!)
and set the start as the prior timer end time. (Notably, it's not set to the block time!)


The timer will tick at the first block whose blocktime is greater than the timer end time,
and set the start as the prior timer end time. (Notably, its not set to the block time!)
This means that if the chain has been down for awhile, you will get one timer tick per block,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace "awhile" with "a while" to correct the phrase.

- if the chain has been down for awhile, you will get one timer tick per block,
+ if the chain has been down for a while, you will get one timer tick per block,

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.

Suggested change
This means that if the chain has been down for awhile, you will get one timer tick per block,
This means that if the chain has been down for a while, you will get one timer tick per block,

github.com/minio/highwayhash v1.0.2 // indirect
)

replace github.com/cosmos/cosmos-sdk => ../../.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The replace directive for github.com/cosmos/cosmos-sdk is used to point to a local path, which is useful for development and testing. Ensure this is adjusted or removed for production releases to avoid dependency resolution issues.

x/epochs/go.mod Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between be9f85f and 6a5f9b4.
Files selected for processing (2)
  • simapp/app.go (8 hunks)
  • simapp/go.mod (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • simapp/app.go
  • simapp/go.mod

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 6a5f9b4 and 8dfe5ca.
Files selected for processing (18)
  • baseapp/cache_ctx.go (1 hunks)
  • simapp/app.go (8 hunks)
  • testutil/list.go (2 hunks)
  • x/epochs/depinject.go (1 hunks)
  • x/epochs/keeper/abci.go (1 hunks)
  • x/epochs/keeper/abci_test.go (1 hunks)
  • x/epochs/keeper/epoch.go (1 hunks)
  • x/epochs/keeper/epoch_test.go (1 hunks)
  • x/epochs/keeper/genesis.go (1 hunks)
  • x/epochs/keeper/genesis_test.go (1 hunks)
  • x/epochs/keeper/grpc_query.go (1 hunks)
  • x/epochs/keeper/keeper.go (1 hunks)
  • x/epochs/keeper/keeper_test.go (1 hunks)
  • x/epochs/module.go (1 hunks)
  • x/epochs/simulation/genesis.go (1 hunks)
  • x/epochs/types/hooks.go (1 hunks)
  • x/epochs/types/hooks_test.go (1 hunks)
  • x/epochs/types/telemetry.go (1 hunks)
Files skipped from review as they are similar to previous changes (16)
  • baseapp/cache_ctx.go
  • simapp/app.go
  • x/epochs/depinject.go
  • x/epochs/keeper/abci.go
  • x/epochs/keeper/abci_test.go
  • x/epochs/keeper/epoch.go
  • x/epochs/keeper/epoch_test.go
  • x/epochs/keeper/genesis.go
  • x/epochs/keeper/genesis_test.go
  • x/epochs/keeper/grpc_query.go
  • x/epochs/keeper/keeper.go
  • x/epochs/keeper/keeper_test.go
  • x/epochs/module.go
  • x/epochs/types/hooks.go
  • x/epochs/types/hooks_test.go
  • x/epochs/types/telemetry.go
Additional comments: 4
testutil/list.go (2)
  • 4-4: The use of math/rand is noted here. While it's common in testing and non-security contexts, ensure that this randomness source is appropriate for your use case. For cryptographic or security-sensitive operations, crypto/rand should be used instead.
  • 19-25: The SortSlice function is a good addition, providing a generic way to sort slices of ordered types. The implementation is correct, and the documentation clearly states its behavior.
x/epochs/simulation/genesis.go (2)
  • 6-6: The use of math/rand here is appropriate given the context of simulation and non-security-sensitive operations. It's important to use crypto/rand for cryptographic or security-sensitive operations, but in this case, math/rand is suitable.
  • 15-46: The functions for generating random epochs and marshalling the genesis state are well-implemented. The logic is sound, and the use of randomness is appropriate for simulation purposes. The error handling in marshalling is correctly done, ensuring robustness.

baseapp/cache_ctx.go Outdated Show resolved Hide resolved
simapp/app.go Outdated Show resolved Hide resolved
simapp/app.go Outdated Show resolved Hide resolved
x/epochs/module.go Outdated Show resolved Hide resolved
return ModuleOutputs{EpochKeeper: k, Module: m}
}

func InvokeSetHooks(keeper keeper.Keeper, hooks map[string]types.EpochHooksWrapper) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed on our standup review, we should add documentation on this #19897

Nothing to do here, just noting it.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 2

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between e0cb398 and ef21944.
Files selected for processing (4)
  • testutil/configurator/configurator.go (4 hunks)
  • x/epochs/keeper/abci.go (1 hunks)
  • x/epochs/module.go (1 hunks)
  • x/epochs/proto/cosmos/epochs/v1beta1/events.proto (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • testutil/configurator/configurator.go
  • x/epochs/keeper/abci.go
  • x/epochs/module.go
Additional comments (1)
x/epochs/proto/cosmos/epochs/v1beta1/events.proto (1)
  • 1-1: The version comment at the top of the file is a good practice for tracking the introduction of features or changes. However, it's essential to ensure that this versioning comment accurately reflects the versioning scheme of the Cosmos SDK or the epochs module specifically. If x/epochs 0.1.0 is not an official or planned version, this could lead to confusion.

Comment on lines +11 to +14
// EventEpochEnd is an event emitted when an epoch end.
message EventEpochEnd {
int64 epoch_number = 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment for EventEpochEnd states it is "an event emitted when an epoch end." To improve clarity and grammatical correctness, consider revising this to "an event emitted when an epoch ends."

-// EventEpochEnd is an event emitted when an epoch end.
+// EventEpochEnd is an event emitted when an epoch ends.

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.

Suggested change
// EventEpochEnd is an event emitted when an epoch end.
message EventEpochEnd {
int64 epoch_number = 1;
}
// EventEpochEnd is an event emitted when an epoch ends.
message EventEpochEnd {
int64 epoch_number = 1;
}

Comment on lines +16 to +21
// EventEpochStart is an event emitted when an epoch start.
message EventEpochStart {

int64 epoch_number = 1;
int64 epoch_start_time = 2;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, for EventEpochStart, the comment could be improved for clarity and grammatical correctness. Consider revising it to "an event emitted when an epoch starts."

-// EventEpochStart is an event emitted when an epoch start.
+// EventEpochStart is an event emitted when an epoch starts.

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.

Suggested change
// EventEpochStart is an event emitted when an epoch start.
message EventEpochStart {
int64 epoch_number = 1;
int64 epoch_start_time = 2;
}
// EventEpochStart is an event emitted when an epoch starts.
message EventEpochStart {
int64 epoch_number = 1;
int64 epoch_start_time = 2;
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between ef21944 and 5826baf.
Files selected for processing (2)
  • simapp/app.go (7 hunks)
  • simapp/go.mod (2 hunks)
Path instructions used (1)
simapp/app.go (1)

**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Files skipped from review as they are similar to previous changes (1)
  • simapp/go.mod
Additional comments (7)
simapp/app.go (7)
  • 53-55: The addition of the epochs module imports is correctly done, ensuring that the necessary components from the epochs module are available for use within simapp/app.go.
  • 173-173: The EpochsKeeper has been added to the SimApp struct as expected, aligning with the PR's objective to integrate the epochs functionality into the Cosmos SDK.
  • 266-266: Adding epochstypes.StoreKey to the NewSimApp function's store keys initialization is necessary for the epochs module to function correctly by providing it with a dedicated key-value store.
  • 416-425: The initialization and setting of hooks for EpochsKeeper are correctly implemented. However, it's noted that the comment suggests adding epoch hooks receivers, which might indicate future work or extensions. Ensure that any necessary hooks are implemented and tested.
  • 451-451: Registering the epochs module in the ModuleManager is a crucial step for integrating the module into the Cosmos SDK, ensuring it participates in the application lifecycle correctly.
  • 472-472: Correctly registering the epochs module to participate in the begin blocker lifecycle event. This is essential for the epochs module to perform its operations at the beginning of each block.
  • 492-492: Including the epochs module in the genesis module order is necessary for initializing the module's state at the chain's genesis. This ensures that the epochs module is correctly set up from the start of the blockchain.

plugins:
- name: gocosmos
out: ..
opt: plugins=grpc,Mgoogle/protobuf/any.proto=github.com/cosmos/cosmos-sdk/codec/types
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that this got merged: #19869, could you update this line to gogoproto any?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review Status

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 5826baf and c3f50e6.
Files ignored due to path filters (1)
  • simapp/go.mod is excluded by !**/*.mod
Files selected for processing (1)
  • simapp/app.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
  • simapp/app.go
Additional Context Used

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, but you still need to re-add the no-op RegisterInterfaces to match the module.AppModule interface.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review Status

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between c3f50e6 and cb194dd.
Files ignored due to path filters (1)
  • x/epochs/types/events.pb.go is excluded by !**/*.pb.go
Files selected for processing (2)
  • api/cosmos/epochs/v1beta1/events.pulsar.go (1 hunks)
  • x/epochs/proto/buf.gen.gogo.yaml (1 hunks)
Files not summarized due to errors (1)
  • api/cosmos/epochs/v1beta1/events.pulsar.go: Error: Message exceeds token limit
Additional Context Used
Path-based Instructions (1)
api/cosmos/epochs/v1beta1/events.pulsar.go (1)

**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (4)
x/epochs/proto/buf.gen.gogo.yaml (1)

5-5: Given the recent merge mentioned in the comments, please ensure that the opt configuration for the gocosmos plugin correctly uses gogoproto any as intended. If the current configuration Mgoogle/protobuf/any.proto=github.com/cosmos/gogoproto/types/any does not align with the new requirements, it may need to be updated accordingly.

api/cosmos/epochs/v1beta1/events.pulsar.go (3)

887-921: The struct definitions for EventEpochEnd and EventEpochStart are well-defined and follow protobuf field naming conventions.


1-1: Ensure that the generated code has not been manually altered, as this could introduce errors. The presence of the comment // Code generated by protoc-gen-go-pulsar. DO NOT EDIT. at the top of the file is a good indicator that the generated code remains untouched.


21-1078: The initialization functions, type definitions, and method implementations related to the protobuf definitions are consistent with standard protobuf generation patterns for Go.

@julienrbrt
Copy link
Member

Can you run make lint-fix and go mod tidy all script?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review Status

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between cb194dd and e5a5459.
Files ignored due to path filters (2)
  • x/epochs/go.mod is excluded by !**/*.mod
  • x/epochs/go.sum is excluded by !**/*.sum
Files selected for processing (1)
  • x/epochs/module.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • x/epochs/module.go
Additional Context Used

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review Status

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between e5a5459 and 7088941.
Files selected for processing (1)
  • x/epochs/types/keys.go (1 hunks)
Additional Context Used
Path-based Instructions (1)
x/epochs/types/keys.go (1)

**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (4)
x/epochs/types/keys.go (4)

1-1: The package declaration follows Go conventions for module-specific types and constants. Good to go.


3-5: The import of cosmossdk.io/collections is correctly used for prefix key management. This is a good practice for modular and maintainable code.


7-13: The constants ModuleName and StoreKey are well-defined, following common patterns in Cosmos SDK modules for consistency and readability.


15-16: The declaration of KeyPrefixEpoch using collections.NewPrefix(1) correctly implements a unique prefix for module-specific keys, adhering to Cosmos SDK best practices and previous feedback.

@facundomedica
Copy link
Member

Please fix the tests so we can merge, I believe it's just go mod stuff

@julienrbrt julienrbrt added this pull request to the merge queue Apr 4, 2024
Merged via the queue into cosmos:main with commit 1028e27 Apr 4, 2024
55 of 58 checks passed
alpe added a commit to alpe/cosmos-sdk that referenced this pull request Apr 8, 2024
* main: (45 commits)
  build(deps): Bump github.com/decred/dcrd/dcrec/secp256k1/v4 from 4.2.0 to 4.3.0 (cosmos#19913)
  build(deps): Bump google.golang.org/grpc from 1.62.1 to 1.63.0 (cosmos#19929)
  test(types/address): add additional unit tests for address.LengthPrefix and a… (cosmos#19964)
  refactor(x/bank)!: remove Address.String() (cosmos#19954)
  build(deps): Bump github.com/prometheus/common from 0.51.1 to 0.52.2 (cosmos#19930)
  fix(x/epochs): Fix init genesis (cosmos#19958)
  feat(core,runtime): transaction service (exec mode) (cosmos#19953)
  fix(x/authz): non utf-8 characters in logs (cosmos#19923)
  build(deps): Bump golang.org/x/crypto from 0.21.0 to 0.22.0 (cosmos#19960)
  chore: fix spelling errors (cosmos#19957)
  fix(x/tx): don't shadow Amino marshalling error messages (cosmos#19955)
  refactor(types): loosen module.AppModule interface (cosmos#19951)
  feat(core): add `TxValidator` interface (cosmos#19950)
  build(deps): Bump cosmossdk.io/store from 1.0.2 to 1.1.0 in /x/epochs (cosmos#19947)
  feat(x/epochs): upstream osmosis epoch module (cosmos#19697)
  build(deps): Bump bufbuild/buf-setup-action from 1.30.0 to 1.30.1 (cosmos#19928)
  refactor(x/genutil)!: remove Address.String() (cosmos#19926)
  docs(x/mint): Fix inconsistency in mint docs  (cosmos#19915)
  build(deps): Bump github.com/regen-network/gocuke from 1.1.0 to 1.1.1 in /orm (cosmos#19920)
  feat: Integrate grpc configuration into client.toml (cosmos#19905)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Upstream Epochs module
5 participants