chore: upstream runtime/v2 #20320
chore: upstream runtime/v2 #20320
Conversation
WalkthroughWalkthroughThe updates across various modules primarily enhance functionality and introduce new features in memory management, data storage, and module interaction. Key enhancements include the addition of new methods for tree management, restructuring of database interactions, and the introduction of a new module manager for streamlined module operations. These changes aim to improve efficiency, extend capabilities, and provide clearer error handling and version management. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Out of diff range and nitpick comments (1)
runtime/v2/manager.go (1)
32-38: Ensure proper documentation for theMMstruct to enhance code maintainability.Consider adding a descriptive comment above the
MMstruct to explain its purpose and main responsibilities within the system. This will improve code readability and maintainability.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
runtime/v2/go.modis excluded by!**/*.modruntime/v2/go.sumis excluded by!**/*.sum
Files selected for processing (10)
- runtime/v2/app.go (1 hunks)
- runtime/v2/builder.go (1 hunks)
- runtime/v2/manager.go (1 hunks)
- runtime/v2/migrations.go (1 hunks)
- runtime/v2/module.go (1 hunks)
- runtime/v2/services/autocli.go (1 hunks)
- runtime/v2/services/comet.go (1 hunks)
- runtime/v2/services/reflection.go (1 hunks)
- runtime/v2/store.go (1 hunks)
- runtime/v2/types.go (1 hunks)
Files skipped from review due to trivial changes (2)
- runtime/v2/services/comet.go
- runtime/v2/types.go
Additional Context Used
Path-based Instructions (8)
runtime/v2/services/reflection.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/migrations.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/services/autocli.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (23)
runtime/v2/services/reflection.go (2)
12-25: The implementation ofReflectionServiceand its constructorNewReflectionServiceare correctly handling the initialization of file descriptors. Good use of error handling in the constructor.
27-31: TheFileDescriptorsmethod correctly constructs and returns the file descriptor response. Proper use of context and error handling.runtime/v2/store.go (2)
10-15: TheNewKVStoreServicefunction is implemented correctly, ensuring backward compatibility by accepting a string key and converting it to bytes.
17-50: TheStoreinterface is well-defined, covering all necessary methods for state management and data querying with clear and consistent method signatures.runtime/v2/migrations.go (3)
12-22: ThemigrationRegistrarstruct and its constructornewMigrationRegistrarare correctly implemented, initializing the migration map appropriately.
24-42: TheRegistermethod inmigrationRegistrarcorrectly handles registration of module migrations, including validation for version numbering and prevention of duplicate registrations.
44-69: TheRunModuleMigrationsmethod is well-implemented, ensuring that migrations are run sequentially and handling cases where no migrations are necessary or where a migration is missing.runtime/v2/services/autocli.go (3)
18-33: TheAutoCLIQueryServicestruct and its constructorNewAutoCLIQueryServiceare correctly implemented, effectively extracting AutoCLI options from app modules and handling errors appropriately.
35-86: TheExtractAutoCLIOptionsmethod is well-implemented, handling the extraction of AutoCLI options from various types of modules with comprehensive error handling and conditional logic.
88-92: TheAppOptionsmethod correctly constructs and returns the AutoCLI options response. Proper use of context and error handling.runtime/v2/app.go (7)
23-143: TheAppstruct and its associated methods likeLogger,ModuleManager,DefaultGenesis, etc., are well-implemented, effectively managing various aspects of the application lifecycle and configurations.
84-92: The methods for managing application state, such asLoadLatestandLoadHeight, are correctly implemented, handling different versions of the application state with appropriate error handling.
94-97: TheClosemethod is correctly implemented for resource cleanup, aligning with the current requirements of the application.
99-112: The methodsRegisterStoresandGetStoreKeysare correctly implemented, handling the registration and retrieval of store keys effectively.
114-123: TheUnsafeFindStoreKeymethod is correctly implemented for testing purposes, using a linear search to find a store key. Caution is advised if used outside of testing due to its linear search time complexity.
125-131: The getter methodsGetStoreandGetLoggerare straightforward and correctly implemented, providing essential access to the application's store and logger.
133-143: The methodsExecuteGenesisTx,SetAppVersion, andAppVersionare placeholders and currently not implemented, throwing a panic. Consider implementing these methods or handling them more gracefully in production environments.runtime/v2/builder.go (3)
20-61: TheAppBuilderstruct and its methods likeDefaultGenesisandRegisterModulesare well-implemented, effectively managing various aspects of application building and configurations.
63-148: TheBuildmethod inAppBuilderis well-implemented, orchestrating the building of an application instance with comprehensive error handling and conditional logic for handling defaults and configurations.
150-167: TheAppBuilderOptionfunctions, such asAppBuilderWithBranchandAppBuilderWithTxValidator, provide effective customization options for the application builder, allowing for setting custom implementations and configurations.runtime/v2/module.go (2)
40-101: TheappModulestruct and its methods likeRegisterServicesandAutoCLIOptionsare well-implemented, effectively managing various aspects of module management and service registration.
103-321: Theinitfunction and associated provider functions inruntime/v2/module.goare correctly implemented, registering various providers and invokers for the application configuration effectively.runtime/v2/manager.go (1)
143-195: Validate the error handling inInitGenesisJSON.The method
InitGenesisJSONhandles various error scenarios, but it's crucial to ensure that all potential errors are appropriately managed and that the error messages are clear and informative. Consider adding tests or checks to verify the robustness of error handling.Verification successful
The verification of the error handling in the
InitGenesisJSONmethod withinmanager.gohas been successfully conducted. The results from the script show a variety of error handling patterns, including the use oferrorsmod.Wrap,errorsmod.Wrapf, and direct returns oferr. These patterns indicate that the method and related code handle errors robustly and provide clear, informative error messages through wrapping errors with additional context.Here are some specific findings:
- Errors are wrapped with context, providing clarity on what the error pertains to, such as "can't begin a none operation" or "failed to create snapshot directory".
- Direct returns of
errare also present, which is typical in Go for propagating errors up the call stack.- The method
InitGenesisJSONspecifically handles a critical error scenario where validator updates are already set by a previous module, returning a new error with a clear message.Based on these observations, the error handling within the
InitGenesisJSONmethod and associated parts ofmanager.goappears to be well-implemented, with comprehensive checks and clear, informative error messages.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in InitGenesisJSON. # Test: Search for error handling patterns. Expect: Comprehensive error checks and clear messages. rg --type go 'return err' 'manager.go'Length of output: 89
Script:
#!/bin/bash # Locate the correct path for manager.go and verify error handling in InitGenesisJSON. fd manager.go --exec rg --type go 'return err' {}Length of output: 3173
kocubinski
force-pushed
the
kocu/runtime-v2
branch
from
1becc15 to
df04a11
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Out of diff range and nitpick comments (1)
server/v2/stf/stf.go (1)
Line range hint
81-81: Avoid usingpanicfor error handling in constructors.Using
panicwithin theNewSTFconstructor (line 81) can lead to abrupt termination of the application. It's generally better practice in Go to return errors from constructors and let the caller handle them gracefully.- panic(err) + return nil, err
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (4)
runtime/v2/go.modis excluded by!**/*.modruntime/v2/go.sumis excluded by!**/*.sumstore/v2/go.modis excluded by!**/*.modstore/v2/go.sumis excluded by!**/*.sum
Files selected for processing (33)
- core/appmodule/v2/genesis.go (1 hunks)
- core/appmodule/v2/message.go (1 hunks)
- core/context/context.go (2 hunks)
- core/transaction/transaction.go (2 hunks)
- go.work.example (2 hunks)
- proto/cosmos/app/runtime/v2/module.proto (1 hunks)
- runtime/v2/app.go (1 hunks)
- runtime/v2/builder.go (1 hunks)
- runtime/v2/manager.go (1 hunks)
- runtime/v2/migrations.go (1 hunks)
- runtime/v2/module.go (1 hunks)
- runtime/v2/services/autocli.go (1 hunks)
- runtime/v2/services/comet.go (1 hunks)
- runtime/v2/services/reflection.go (1 hunks)
- runtime/v2/services/transaction.go (1 hunks)
- runtime/v2/store.go (1 hunks)
- runtime/v2/types.go (1 hunks)
- server/v2/stf/core_router_service.go (1 hunks)
- server/v2/stf/export_test.go (1 hunks)
- server/v2/stf/mock/tx.go (1 hunks)
- server/v2/stf/stf.go (1 hunks)
- server/v2/stf/stf_router.go (2 hunks)
- store/v2/commitment/mem/tree.go (1 hunks)
- store/v2/commitment/store.go (8 hunks)
- store/v2/db/db.go (1 hunks)
- store/v2/db/db_test.go (1 hunks)
- store/v2/errors/errors.go (1 hunks)
- store/v2/internal/util.go (1 hunks)
- store/v2/root/factory.go (1 hunks)
- store/v2/root/store.go (1 hunks)
- store/v2/storage/pebbledb/db.go (1 hunks)
- store/v2/storage/rocksdb/db.go (1 hunks)
- store/v2/storage/sqlite/db.go (3 hunks)
Files skipped from review due to trivial changes (3)
- go.work.example
- server/v2/stf/export_test.go
- store/v2/root/store.go
Files skipped from review as they are similar to previous changes (9)
- runtime/v2/app.go
- runtime/v2/builder.go
- runtime/v2/migrations.go
- runtime/v2/module.go
- runtime/v2/services/autocli.go
- runtime/v2/services/comet.go
- runtime/v2/services/reflection.go
- runtime/v2/store.go
- runtime/v2/types.go
Additional Context Used
Path-based Instructions (20)
store/v2/internal/util.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/context/context.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/appmodule/v2/message.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/db/db.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/appmodule/v2/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/services/transaction.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/transaction/transaction.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/commitment/mem/tree.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/errors/errors.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/mock/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/db/db_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"store/v2/root/factory.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/core_router_service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/stf_router.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/rocksdb/db.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/sqlite/db.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/pebbledb/db.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/commitment/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/stf.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (19)
store/v2/internal/util.go (1)
5-7: The implementation ofIsMemoryStoreKeyis concise and correctly checks for the "memory:" prefix in keys. Good use ofstrings.HasPrefixfor this purpose.core/context/context.go (1)
Line range hint
1-21: The definitions ofExecModeand related constants are well-implemented. UsingiotaforExecModeKeyensures uniqueness and clarity in its usage within contexts.core/appmodule/v2/message.go (1)
Line range hint
8-14: The implementation ofmessageNamefunction and the type aliasMessageare well-designed. The type switch inmessageNameis effectively used to handle different message types, ensuring robustness.store/v2/db/db.go (1)
25-26: Please add a comment explaining why theDBTypeRocksDBcase is commented out. This will help maintain clarity and prevent confusion about the support status of RocksDB.core/appmodule/v2/genesis.go (1)
18-20: The introduction of theHasABCIGenesisinterface is well-thought-out, providing clear and modular handling of genesis-related operations, especially with the ability to returnValidatorUpdatefromInitGenesis.core/transaction/transaction.go (1)
25-25: Please ensure that the TODO comment inGetSendersis tracked in an issue or resolved. It's important to have clarity on whether the method will be adjusted to return a single identity.store/v2/errors/errors.go (2)
46-47: The addition ofRequestedVersionandEarliestVersionfields in theErrVersionPrunedstruct enhances error descriptiveness.
51-51: The error message now effectively utilizes the new fields to provide a clearer context on version pruning issues.server/v2/stf/mock/tx.go (1)
25-26: The implementation ofGetMessagescorrectly returns the transaction's message in a slice.store/v2/db/db_test.go (1)
109-118: TheTestRocksDBSuiteis commented out with a note to re-enable it behind a build flag. Verify the reason for this and consider tracking this as an issue or TODO in the project.proto/cosmos/app/runtime/v2/module.proto (1)
7-56: The protobuf messageModuleis well-defined with clear configurations for runtime modules.store/v2/root/factory.go (1)
41-101: TheCreateRootStorefunction is well-implemented, demonstrating the necessary steps and configurations for creating a root store based on the provided options.server/v2/stf/core_router_service.go (1)
15-37: TheNewRouterServicefunction correctly initializes a router service with separate handlers for messages and queries, using appropriate error handling.server/v2/stf/stf_router.go (1)
57-60: TheHandlerExistsmethod correctly checks for the existence of a handler for a given message type.store/v2/storage/rocksdb/db.go (1)
102-102: ThegetSlicemethod appropriately handles version checks and uses theErrVersionPrunederror to report issues effectively.store/v2/storage/sqlite/db.go (1)
142-142: Enhanced error descriptiveness in version pruning is a good improvement.store/v2/storage/pebbledb/db.go (1)
152-152: Enhanced error descriptiveness in version pruning is a good improvement.store/v2/commitment/store.go (2)
48-53: Initialization of default pruning options when none are provided enhances robustness.
92-94: Skipping memory store keys in commit info generation aligns with best practices for data persistence.
| @@ -290,7 +290,7 @@ func (s STF[T]) runTxMsgs( | |||
| if err != nil { | |||
| return nil, 0, nil, err | |||
| } | |||
| msgs := tx.GetMessages() | |||
| msgs, err := tx.GetMessages() | |||
There was a problem hiding this comment.
Consider handling errors more gracefully in runTxMsgs.
The error handling in the runTxMsgs method could be improved. Currently, if an error occurs when fetching messages with tx.GetMessages(), the method returns immediately. This might leave some cleanup or state finalization steps skipped, which could lead to inconsistent state or resource leaks. Consider adding a deferred function to handle such cleanup tasks.
+ defer func() {
+ if err != nil {
+ // Perform necessary cleanup
+ }
+ }()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.
| msgs, err := tx.GetMessages() | |
| msgs, err := tx.GetMessages() | |
| defer func() { | |
| if err != nil { | |
| // Perform necessary cleanup | |
| } | |
| }() |
Optimize error handling in message processing.
In the runTxMsgs function, the error handling could be optimized by adding a deferred cleanup or rollback mechanism. This would ensure that any resources are properly released or states are reverted in case of an error during message processing.
+ defer func() {
+ if err != nil {
+ // Cleanup or rollback
+ }
+ }()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.
| msgs, err := tx.GetMessages() | |
| msgs, err := tx.GetMessages() | |
| defer func() { | |
| if err != nil { | |
| // Cleanup or rollback | |
| } | |
| }() |
| @@ -84,7 +84,7 @@ func New(dataDir string) (*Database, error) { | |||
|
|
|||
| return &Database{ | |||
| storage: storage, | |||
| earliestVersion: pruneHeight + 1, | |||
| earliestVersion: pruneHeight, | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The analysis of the earliestVersion usage in the codebase suggests a potential issue with its initialization. The variable is used to determine if a targetVersion is pruned by comparing it against earliestVersion. Additionally, earliestVersion is updated to version + 1 elsewhere in the code, indicating that it should be one more than the last pruned version. Initializing it directly to pruneHeight could lead to versions being considered pruned earlier than intended. This discrepancy needs attention:
- File:
store/v2/storage/sqlite/db.go- Line 87: Initialization of
earliestVersiontopruneHeightmight be incorrect. - Line 102:
earliestVersionis set toversion + 1, which conflicts with the initialization logic.
- Line 87: Initialization of
Analysis chain
Verify the change in earliestVersion initialization.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the intended behavior of `earliestVersion` initialization change.
# Test: Search for the usage of `earliestVersion`. Expect: Consistent usage considering the new initialization.
rg --type go $'earliestVersion' -- 'store/v2/storage/sqlite/'
Length of output: 638
| @@ -60,7 +66,7 @@ func NewCommitStore(trees map[string]Tree, db store.RawDB, pruneOpts *store.Prun | |||
| func (c *CommitStore) WriteBatch(cs *corestore.Changeset) error { | |||
| for _, pairs := range cs.Changes { | |||
|
|
|||
| key := internal.UnsafeBytesToStr(pairs.Actor) | |||
| key := conv.UnsafeBytesToStr(pairs.Actor) | |||
There was a problem hiding this comment.
Consider using a safer method for byte-to-string conversion to avoid potential issues.
- key := conv.UnsafeBytesToStr(pairs.Actor)
+ key := string(pairs.Actor)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.
| key := conv.UnsafeBytesToStr(pairs.Actor) | |
| key := string(pairs.Actor) |
|
|
||
| // ExecMode returns the execution mode stored in the context. | ||
| func (c ContextAwareTransactionService) ExecMode(ctx context.Context) transaction.ExecMode { | ||
| return ctx.Value(corecontext.ExecModeKey).(transaction.ExecMode) |
There was a problem hiding this comment.
Consider adding error handling for the type assertion in ExecMode. This will prevent potential runtime panics if ExecModeKey is not present in the context or if it is not of type transaction.ExecMode.
- return ctx.Value(corecontext.ExecModeKey).(transaction.ExecMode)
+ execMode, ok := ctx.Value(corecontext.ExecModeKey).(transaction.ExecMode)
+ if !ok {
+ return transaction.ExecMode{}, fmt.Errorf("exec mode not found in context")
+ }
+ return execModeCommittable 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.
| return ctx.Value(corecontext.ExecModeKey).(transaction.ExecMode) | |
| execMode, ok := ctx.Value(corecontext.ExecModeKey).(transaction.ExecMode) | |
| if !ok { | |
| return transaction.ExecMode{}, fmt.Errorf("exec mode not found in context") | |
| } | |
| return execMode |
runtime/v2/go.mod
Outdated
| cosmossdk.io/x/auth => ../../x/auth | ||
| cosmossdk.io/x/authz => ../../x/authz | ||
| cosmossdk.io/x/bank => ../../x/bank | ||
| cosmossdk.io/x/circuit => ../../x/circuit | ||
| cosmossdk.io/x/consensus => ../../x/consensus | ||
| cosmossdk.io/x/distribution => ../../x/distribution | ||
| cosmossdk.io/x/evidence => ../../x/evidence | ||
| cosmossdk.io/x/feegrant => ../../x/feegrant | ||
| cosmossdk.io/x/gov => ../../x/gov | ||
| cosmossdk.io/x/group => ../../x/group | ||
| cosmossdk.io/x/mint => ../../x/mint | ||
| cosmossdk.io/x/nft => ../../x/nft | ||
| cosmossdk.io/x/params => ../../x/params | ||
| cosmossdk.io/x/protocolpool => ../../x/protocolpool | ||
| cosmossdk.io/x/slashing => ../../x/slashing | ||
| cosmossdk.io/x/staking => ../../x/staking |
| type ReflectionService struct { | ||
| reflectionv1.UnimplementedReflectionServiceServer | ||
| files *descriptorpb.FileDescriptorSet | ||
| } | ||
|
|
||
| func NewReflectionService() (*ReflectionService, error) { | ||
| fds, err := proto.MergedGlobalFileDescriptors() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &ReflectionService{files: fds}, nil | ||
| } | ||
|
|
||
| func (r ReflectionService) FileDescriptors(_ context.Context, _ *reflectionv1.FileDescriptorsRequest) (*reflectionv1.FileDescriptorsResponse, error) { | ||
| return &reflectionv1.FileDescriptorsResponse{ | ||
| Files: r.files.File, | ||
| }, nil | ||
| } | ||
|
|
||
| var _ reflectionv1.ReflectionServiceServer = &ReflectionService{} |
There was a problem hiding this comment.
taking note for later, but I believe we should move this out of runtime. There isn't a need for this to live in runtime. It would be the underlying server that would set this up
| // InvokeTyped execute a message and fill-in a response. | ||
| // The response must be known and passed as a parameter. | ||
| // Use InvokeUntyped if the response type is not known. | ||
| func (m *msgRouterService) InvokeTyped(ctx context.Context, msg, resp protoiface.MessageV1) error { |
There was a problem hiding this comment.
| func (m *msgRouterService) InvokeTyped(ctx context.Context, msg, resp protoiface.MessageV1) error { | |
| func (m *msgRouterService) InvokeTyped(ctx context.Context, msg, resp *protoiface.MessageV1) error { |
isn't this suppose to be a pointer?
There was a problem hiding this comment.
I didn't give it much thought because we have the same usage in runtime/v1 router, but I don't think it will work as intended even as a pointer. That usage only works there due to the proto.Merge usages in the hybrid router I believe.
| } | ||
|
|
||
| // InvokeUntyped execute a message and returns a response. | ||
| func (m *msgRouterService) InvokeUntyped(ctx context.Context, msg protoiface.MessageV1) (protoiface.MessageV1, error) { |
There was a problem hiding this comment.
should msg everywhere be transaction.Msg?
There was a problem hiding this comment.
probably, OK to break core for this?
https://github.com/cosmos/cosmos-sdk/blob/kocu/runtime-v2/core/router/service.go#L16-L24
| var _ commitment.Tree = (*Tree)(nil) | ||
|
|
||
| // Tree is a simple in-memory implementation of commitment.Tree. | ||
| type Tree struct { |
There was a problem hiding this comment.
replicates the usage of memory stores in store/server v1, store v2 usage: https://github.com/cosmos/cosmos-sdk/blob/kocu/runtime-v2/runtime/v2/module.go#L241-L243
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (6)
- runtime/v2/manager.go (1 hunks)
- server/v2/appmanager/config.go (1 hunks)
- server/v2/stf/core_router_service.go (1 hunks)
- server/v2/stf/stf_router.go (1 hunks)
- store/v2/db/db.go (1 hunks)
- store/v2/root/factory.go (1 hunks)
Files skipped from review due to trivial changes (1)
- server/v2/appmanager/config.go
Files skipped from review as they are similar to previous changes (4)
- server/v2/stf/core_router_service.go
- server/v2/stf/stf_router.go
- store/v2/db/db.go
- store/v2/root/factory.go
Additional Context Used
Path-based Instructions (1)
runtime/v2/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
| // NewModuleManager is the constructor for the module manager | ||
| // It handles all the interactions between the modules and the application | ||
| func NewModuleManager( | ||
| logger log.Logger, | ||
| cdc codec.Codec, | ||
| config *runtimev2.Module, | ||
| modules map[string]appmodulev2.AppModule, | ||
| ) *MM { | ||
| // good defaults for the module manager order | ||
| modulesName := maps.Keys(modules) | ||
| if len(config.PreBlockers) == 0 { | ||
| config.PreBlockers = modulesName | ||
| } | ||
| if len(config.BeginBlockers) == 0 { | ||
| config.BeginBlockers = modulesName | ||
| } | ||
| if len(config.EndBlockers) == 0 { | ||
| config.EndBlockers = modulesName | ||
| } | ||
| if len(config.TxValidators) == 0 { | ||
| config.TxValidators = modulesName | ||
| } | ||
| if len(config.InitGenesis) == 0 { | ||
| config.InitGenesis = modulesName | ||
| } | ||
| if len(config.ExportGenesis) == 0 { | ||
| config.ExportGenesis = modulesName | ||
| } | ||
| if len(config.OrderMigrations) == 0 { | ||
| config.OrderMigrations = sdkmodule.DefaultMigrationsOrder(modulesName) | ||
| } | ||
|
|
||
| mm := &MM{ | ||
| logger: logger, | ||
| cdc: cdc, | ||
| config: config, | ||
| modules: modules, | ||
| migrationRegistrar: newMigrationRegistrar(), | ||
| } | ||
|
|
||
| if err := mm.validateConfig(); err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| return mm |
There was a problem hiding this comment.
Consider replacing panic with error handling in the constructor.
Using panic for error handling in the constructor (line 80) is aggressive and could lead to abrupt termination of the application. Consider returning the error to the caller to allow for more graceful error handling.
- func NewModuleManager(...) *MM {
+ func NewModuleManager(...) (*MM, error) {
- panic(err)
+ return nil, err
- return mm
+ return mm, nilCommittable 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.
| // NewModuleManager is the constructor for the module manager | |
| // It handles all the interactions between the modules and the application | |
| func NewModuleManager( | |
| logger log.Logger, | |
| cdc codec.Codec, | |
| config *runtimev2.Module, | |
| modules map[string]appmodulev2.AppModule, | |
| ) *MM { | |
| // good defaults for the module manager order | |
| modulesName := maps.Keys(modules) | |
| if len(config.PreBlockers) == 0 { | |
| config.PreBlockers = modulesName | |
| } | |
| if len(config.BeginBlockers) == 0 { | |
| config.BeginBlockers = modulesName | |
| } | |
| if len(config.EndBlockers) == 0 { | |
| config.EndBlockers = modulesName | |
| } | |
| if len(config.TxValidators) == 0 { | |
| config.TxValidators = modulesName | |
| } | |
| if len(config.InitGenesis) == 0 { | |
| config.InitGenesis = modulesName | |
| } | |
| if len(config.ExportGenesis) == 0 { | |
| config.ExportGenesis = modulesName | |
| } | |
| if len(config.OrderMigrations) == 0 { | |
| config.OrderMigrations = sdkmodule.DefaultMigrationsOrder(modulesName) | |
| } | |
| mm := &MM{ | |
| logger: logger, | |
| cdc: cdc, | |
| config: config, | |
| modules: modules, | |
| migrationRegistrar: newMigrationRegistrar(), | |
| } | |
| if err := mm.validateConfig(); err != nil { | |
| panic(err) | |
| } | |
| return mm | |
| // NewModuleManager is the constructor for the module manager | |
| // It handles all the interactions between the modules and the application | |
| func NewModuleManager( | |
| logger log.Logger, | |
| cdc codec.Codec, | |
| config *runtimev2.Module, | |
| modules map[string]appmodulev2.AppModule, | |
| ) (*MM, error) { | |
| // good defaults for the module manager order | |
| modulesName := maps.Keys(modules) | |
| if len(config.PreBlockers) == 0 { | |
| config.PreBlockers = modulesName | |
| } | |
| if len(config.BeginBlockers) == 0 { | |
| config.BeginBlockers = modulesName | |
| } | |
| if len(config.EndBlockers) == 0 { | |
| config.EndBlockers = modulesName | |
| } | |
| if len(config.TxValidators) == 0 { | |
| config.TxValidators = modulesName | |
| } | |
| if len(config.InitGenesis) == 0 { | |
| config.InitGenesis = modulesName | |
| } | |
| if len(config.ExportGenesis) == 0 { | |
| config.ExportGenesis = modulesName | |
| } | |
| if len(config.OrderMigrations) == 0 { | |
| config.OrderMigrations = sdkmodule.DefaultMigrationsOrder(modulesName) | |
| } | |
| mm := &MM{ | |
| logger: logger, | |
| cdc: cdc, | |
| config: config, | |
| modules: modules, | |
| migrationRegistrar: newMigrationRegistrar(), | |
| } | |
| if err := mm.validateConfig(); err != nil { | |
| return nil, err | |
| } | |
| return mm, nil | |
| } |
| // RegisterLegacyAminoCodec registers all module codecs | ||
| func (m *MM) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) { | ||
| for _, b := range m.modules { | ||
| if mod, ok := b.(sdkmodule.HasAminoCodec); ok { | ||
| mod.RegisterLegacyAminoCodec(cdc) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimize the codec registration loop.
The current implementation checks each module individually to see if it implements the HasAminoCodec interface. This could be optimized by pre-filtering modules that implement this interface.
+ filteredModules := filterModulesWithAminoCodec(m.modules)
- for _, b := range m.modules {
+ for _, b := range filteredModules {
if mod, ok := b.(sdkmodule.HasAminoCodec); ok {
mod.RegisterLegacyAminoCodec(cdc)
}
}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.
| // RegisterLegacyAminoCodec registers all module codecs | |
| func (m *MM) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) { | |
| for _, b := range m.modules { | |
| if mod, ok := b.(sdkmodule.HasAminoCodec); ok { | |
| mod.RegisterLegacyAminoCodec(cdc) | |
| } | |
| } | |
| } | |
| // RegisterLegacyAminoCodec registers all module codecs | |
| func (m *MM) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) { | |
| filteredModules := filterModulesWithAminoCodec(m.modules) | |
| for _, b := range filteredModules { | |
| if mod, ok := b.(sdkmodule.HasAminoCodec); ok { | |
| mod.RegisterLegacyAminoCodec(cdc) | |
| } | |
| } | |
| } |
| // ExportGenesisForModules performs export genesis functionality for modules | ||
| func (m *MM) ExportGenesisForModules(ctx context.Context, modulesToExport ...string) (map[string]json.RawMessage, error) { | ||
| if len(modulesToExport) == 0 { | ||
| modulesToExport = m.config.ExportGenesis | ||
| } | ||
| // verify modules exists in app, so that we don't panic in the middle of an export | ||
| if err := m.checkModulesExists(modulesToExport); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| type genesisResult struct { | ||
| bz json.RawMessage | ||
| err error | ||
| } | ||
|
|
||
| type ModuleI interface { | ||
| ExportGenesis(ctx context.Context) (json.RawMessage, error) | ||
| } | ||
|
|
||
| channels := make(map[string]chan genesisResult) | ||
| for _, moduleName := range modulesToExport { | ||
| mod := m.modules[moduleName] | ||
| var moduleI ModuleI | ||
|
|
||
| if module, hasGenesis := mod.(appmodulev2.HasGenesis); hasGenesis { | ||
| moduleI = module.(ModuleI) | ||
| } else if module, hasABCIGenesis := mod.(appmodulev2.HasGenesis); hasABCIGenesis { | ||
| moduleI = module.(ModuleI) | ||
| } | ||
|
|
||
| channels[moduleName] = make(chan genesisResult) | ||
| go func(moduleI ModuleI, ch chan genesisResult) { | ||
| jm, err := moduleI.ExportGenesis(ctx) | ||
| if err != nil { | ||
| ch <- genesisResult{nil, err} | ||
| return | ||
| } | ||
| ch <- genesisResult{jm, nil} | ||
| }(moduleI, channels[moduleName]) | ||
| } | ||
|
|
||
| genesisData := make(map[string]json.RawMessage) | ||
| for moduleName := range channels { | ||
| res := <-channels[moduleName] | ||
| if res.err != nil { | ||
| return nil, fmt.Errorf("genesis export error in %s: %w", moduleName, res.err) | ||
| } | ||
|
|
||
| genesisData[moduleName] = res.bz | ||
| } | ||
|
|
||
| return genesisData, nil | ||
| } |
There was a problem hiding this comment.
Limit the number of concurrent goroutines in ExportGenesisForModules.
The method ExportGenesisForModules uses goroutines to parallelize the export process but does not limit the number of concurrent goroutines, which could lead to resource exhaustion. Consider using a worker pool or limiting the number of concurrent goroutines.
+ const maxGoroutines = 10
+ semaphore := make(chan struct{}, maxGoroutines)
for _, moduleName := range modulesToExport {
+ semaphore <- struct{}{}
go func(moduleI ModuleI, ch chan genesisResult) {
+ defer func() { <-semaphore }()
jm, err := moduleI.ExportGenesis(ctx)
if err != nil {
ch <- genesisResult{nil, err}
return
}
ch <- genesisResult{jm, nil}
}(moduleI, channels[moduleName])
}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.
| // ExportGenesisForModules performs export genesis functionality for modules | |
| func (m *MM) ExportGenesisForModules(ctx context.Context, modulesToExport ...string) (map[string]json.RawMessage, error) { | |
| if len(modulesToExport) == 0 { | |
| modulesToExport = m.config.ExportGenesis | |
| } | |
| // verify modules exists in app, so that we don't panic in the middle of an export | |
| if err := m.checkModulesExists(modulesToExport); err != nil { | |
| return nil, err | |
| } | |
| type genesisResult struct { | |
| bz json.RawMessage | |
| err error | |
| } | |
| type ModuleI interface { | |
| ExportGenesis(ctx context.Context) (json.RawMessage, error) | |
| } | |
| channels := make(map[string]chan genesisResult) | |
| for _, moduleName := range modulesToExport { | |
| mod := m.modules[moduleName] | |
| var moduleI ModuleI | |
| if module, hasGenesis := mod.(appmodulev2.HasGenesis); hasGenesis { | |
| moduleI = module.(ModuleI) | |
| } else if module, hasABCIGenesis := mod.(appmodulev2.HasGenesis); hasABCIGenesis { | |
| moduleI = module.(ModuleI) | |
| } | |
| channels[moduleName] = make(chan genesisResult) | |
| go func(moduleI ModuleI, ch chan genesisResult) { | |
| jm, err := moduleI.ExportGenesis(ctx) | |
| if err != nil { | |
| ch <- genesisResult{nil, err} | |
| return | |
| } | |
| ch <- genesisResult{jm, nil} | |
| }(moduleI, channels[moduleName]) | |
| } | |
| genesisData := make(map[string]json.RawMessage) | |
| for moduleName := range channels { | |
| res := <-channels[moduleName] | |
| if res.err != nil { | |
| return nil, fmt.Errorf("genesis export error in %s: %w", moduleName, res.err) | |
| } | |
| genesisData[moduleName] = res.bz | |
| } | |
| return genesisData, nil | |
| } | |
| // ExportGenesisForModules performs export genesis functionality for modules | |
| func (m *MM) ExportGenesisForModules(ctx context.Context, modulesToExport ...string) (map[string]json.RawMessage, error) { | |
| if len(modulesToExport) == 0 { | |
| modulesToExport = m.config.ExportGenesis | |
| } | |
| // verify modules exists in app, so that we don't panic in the middle of an export | |
| if err := m.checkModulesExists(modulesToExport); err != nil { | |
| return nil, err | |
| } | |
| type genesisResult struct { | |
| bz json.RawMessage | |
| err error | |
| } | |
| type ModuleI interface { | |
| ExportGenesis(ctx context.Context) (json.RawMessage, error) | |
| } | |
| channels := make(map[string]chan genesisResult) | |
| const maxGoroutines = 10 | |
| semaphore := make(chan struct{}, maxGoroutines) | |
| for _, moduleName := range modulesToExport { | |
| mod := m.modules[moduleName] | |
| var moduleI ModuleI | |
| if module, hasGenesis := mod.(appmodulev2.HasGenesis); hasGenesis { | |
| moduleI = module.(ModuleI) | |
| } else if module, hasABCIGenesis := mod.(appmodulev2.HasGenesis); hasABCIGenesis { | |
| moduleI = module.(ModuleI) | |
| } | |
| channels[moduleName] = make(chan genesisResult) | |
| semaphore <- struct{}{} | |
| go func(moduleI ModuleI, ch chan genesisResult) { | |
| defer func() { <-semaphore }() | |
| jm, err := moduleI.ExportGenesis(ctx) | |
| if err != nil { | |
| ch <- genesisResult{nil, err} | |
| return | |
| } | |
| ch <- genesisResult{jm, nil} | |
| }(moduleI, channels[moduleName]) | |
| } | |
| genesisData := make(map[string]json.RawMessage) | |
| for moduleName := range channels { | |
| res := <-channels[moduleName] | |
| if res.err != nil { | |
| return nil, fmt.Errorf("genesis export error in %s: %w", moduleName, res.err) | |
| } | |
| genesisData[moduleName] = res.bz | |
| } | |
| return genesisData, nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (3)
runtime/v2/go.modis excluded by!**/*.modstore/v2/go.modis excluded by!**/*.modstore/v2/go.sumis excluded by!**/*.sum
Files selected for processing (1)
- server/v2/stf/core_router_service.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/stf/core_router_service.go
| @@ -1,11 +1,11 @@ | |||
| package transaction | |||
|
|
|||
| import ( | |||
| "github.com/cosmos/gogoproto/proto" | |||
| gogoproto "github.com/cosmos/gogoproto/proto" | |||
There was a problem hiding this comment.
@tac0turtle is it possible to decouple
cosmossdk.io/core and gogoproto dependency tree wise?
There was a problem hiding this comment.
Gogoproto is already a type within core. Since it's directly related to how the system works it's not needed to decouple. This import also does not prevent compiling down to rv32. What is the reason you would want to avoid this?
There was a problem hiding this comment.
Don't see why it can't be
type Marshallalable interface {
Marshal()
Unmarshal()
}
Having core abstracted from any implementation details around serialization seems like good practice.
cosmossdk.io/core should be over the wire agnostic imo.
There was a problem hiding this comment.
If we continue on the path of enshrining protobuf into base types of the system we will never get rid of the nightmares.
There was a problem hiding this comment.
proto.message is an interface,
type Message interface {
Reset()
String() string
ProtoMessage()
}its annoying I understand but this isn't defining serialisation either.
Marshal and unmarshal dont work here since you cant assume a type has those
| cosmossdk.io/x/accounts => ../../x/accounts | ||
| cosmossdk.io/x/auth => ../../x/auth | ||
| cosmossdk.io/x/bank => ../../x/bank | ||
| cosmossdk.io/x/consensus => ../../x/consensus | ||
| cosmossdk.io/x/distribution => ../../x/distribution | ||
| cosmossdk.io/x/staking => ../../x/staking | ||
| cosmossdk.io/x/tx => ../../x/tx |
There was a problem hiding this comment.
This coupling feels like a bad smell as well.
There was a problem hiding this comment.
these module imports are 2nd order (indirect) from importing github.com/cosmos/cosmos-sdk which is the root code smell. I think the only way to avoid this is the creation of a codec/v2/go.mod to decouple the codec package usages. @tac0turtle wdyt?
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- runtime/v2/app.go (1 hunks)
- runtime/v2/builder.go (1 hunks)
- runtime/v2/module.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- runtime/v2/app.go
- runtime/v2/module.go
Additional Context Used
Path-based Instructions (1)
runtime/v2/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (6)
runtime/v2/builder.go (6)
33-36: MethodDefaultGenesiscorrectly retrieves the default genesis configuration from the module manager.
62-71: MethodRegisterStorescorrectly handles appending provided store keys, with proper nil checks onstoreOptions.
73-160: MethodBuildcorrectly constructs anAppinstance with comprehensive error handling and default settings.
166-171: MethodAppBuilderWithBranchcorrectly implements the functional option pattern for setting a custom branch implementation.
173-179: MethodAppBuilderWithTxValidatorcorrectly implements the functional option pattern for setting a custom transaction validator.
181-187: MethodAppBuilderWithPostTxExeccorrectly implements the functional option pattern for setting custom post-transaction execution logic.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- runtime/v2/app.go (1 hunks)
- runtime/v2/builder.go (1 hunks)
- runtime/v2/store.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- runtime/v2/app.go
- runtime/v2/builder.go
- runtime/v2/store.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- store/v2/commitment/store.go (8 hunks)
- store/v2/db/db.go (1 hunks)
- store/v2/root/store.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- store/v2/commitment/store.go
- store/v2/db/db.go
- store/v2/root/store.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- runtime/v2/module.go (1 hunks)
- store/v2/root/factory.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- runtime/v2/module.go
- store/v2/root/factory.go
Description
Upstream runtime/v2 used in server/v2 development branches. Depends on: #20315, #20286
Also brings in some store/v2 bug fixes and required features discovered in the simapp/v2 integration (so far).
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
New Features
Enhancements
Refactor
Documentation