refactor(simapp,runtime): audit changes #21310
Conversation
WalkthroughWalkthroughThe changes introduce significant updates to the Cosmos SDK, particularly in transaction handling by supporting unordered transactions, enhancing application configuration flexibility, and refining resource management. Developers are required to adjust transaction submission logic and transition legacy applications to the new unordered transaction management system. Additionally, there are modifications to various struct fields and methods across multiple files, aimed at improving clarity and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AppBuilder
participant UnorderedTxManager
participant App
Client->>AppBuilder: Submit transaction with unordered flag
AppBuilder->>UnorderedTxManager: Register unordered transaction manager
UnorderedTxManager->>App: Process transaction
App->>Client: Return transaction result
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 Documentation and Community
|
julienrbrt
added
the
backport/v0.52.x
julienrbrt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
UPGRADING.md (3)
122-124: Improve grammar for clarity.The segment can be improved for better readability. Consider this revision:
- The Cosmos SDK now supports unordered transactions. This means that transactions - can be executed in any order and doesn't require the client to deal with or manage - nonces. This also means the order of execution is not guaranteed. + The Cosmos SDK now supports unordered transactions. This means that transactions + can be executed in any order and do not require the client to manage nonces. + Consequently, the order of execution is not guaranteed.
Line range hint
125-191: Improve grammar for clarity.The segment can be improved for better readability. Consider this revision:
- If you are still using the legacy wiring, you must enable unordered transactions manually: + If you are still using legacy wiring, you must manually enable unordered transactions: - * Update the `App` constructor to create, load, and save the unordered transaction - manager. + * Update the `App` constructor to create, load, and save the unordered transaction manager. - * Add the decorator to the existing AnteHandler chain, which should be as early - as possible. + * Add the decorator to the existing AnteHandler chain as early as possible. - * If the App has a SnapshotManager defined, you must also register the extension - for the TxManager. + * If the App has a SnapshotManager defined, you must also register the extension for the TxManager. - * Create or update the App's `Close()` method to close the unordered tx manager. - Note, this is critical as it ensures the manager's state is written to file - such that when the node restarts, it can recover the state to provide replay - protection. + * Create or update the App's `Close()` method to close the unordered tx manager. + This is critical as it ensures the manager's state is written to a file, + allowing the node to recover the state and provide replay protection upon restart.
Line range hint
192-197: Improve grammar for clarity.The segment can be improved for better readability. Consider this revision:
- To submit an unordered transaction, the client must set the `unordered` flag to - `true` and ensure a reasonable `timeout_height` is set. The `timeout_height` is - used as a TTL for the transaction and is used to provide replay protection. See - [ADR-070](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-070-unordered-transactions.md) - for more details. + To submit an unordered transaction, the client must set the `unordered` flag to `true` + and ensure a reasonable `timeout_height` is set. The `timeout_height` serves as a TTL + for the transaction and provides replay protection. See + [ADR-070](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-070-unordered-transactions.md) + for more details.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (11)
- UPGRADING.md (2 hunks)
- runtime/app.go (4 hunks)
- runtime/builder.go (3 hunks)
- runtime/module.go (4 hunks)
- runtime/v2/builder.go (1 hunks)
- runtime/v2/module.go (1 hunks)
- server/util.go (1 hunks)
- simapp/app_config.go (1 hunks)
- simapp/app_di.go (7 hunks)
- simapp/simd/cmd/config.go (1 hunks)
- simapp/v2/app_config.go (1 hunks)
Files skipped from review due to trivial changes (4)
- runtime/v2/module.go
- server/util.go
- simapp/app_config.go
- simapp/v2/app_config.go
Additional context used
Path-based instructions (7)
runtime/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/simd/cmd/config.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/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.UPGRADING.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (16)
runtime/builder.go (5)
26-28: LGTM!The addition of
appOptionsenhances the flexibility of the configuration options.The code changes are approved.
53-63: LGTM!The changes introduce new functionalities and control flows. The error handling is appropriate, and the logic is clear.
The code changes are approved.
73-85: LGTM!The method correctly initializes and starts the unordered transaction manager. The error handling is appropriate.
The code changes are approved.
87-101: LGTM!The method correctly handles the registration of the indexer and legacy streaming services based on the configuration options.
The code changes are approved.
103-112: LGTM!The method correctly retrieves and filters key-value store keys.
The code changes are approved.
simapp/simd/cmd/config.go (1)
Line range hint
1-1: Verify the impact of the change.The removal of the line disabling the fast node feature simplifies the initialization process but may impact performance or resource usage. Ensure this change aligns with the application's requirements.
runtime/v2/builder.go (1)
128-135: LGTM!The refactoring improves readability and maintains the overall logic flow. The changes are appropriate.
The code changes are approved.
runtime/module.go (3)
149-149: LGTM!The initialization of
AppBuilderwith the namedappfield improves clarity.The code changes are approved.
164-164: LGTM!The addition of the
AppOptionsfield increases flexibility in client wiring.The code changes are approved.
176-178: LGTM!The check for
AppOptionsensures that the application can utilize provided options if available.The code changes are approved.
runtime/app.go (3)
46-46: LGTM!The addition of the
UnorderedTxManagerfield enhances the application's transaction handling capabilities.The code changes are approved.
162-168: LGTM!The
Closefunction ensures proper resource management and stability during the shutdown process.The code changes are approved.
275-285: LGTM!The
GetKeyfunction provides a controlled way for developers to access store keys during testing.The code changes are approved.
simapp/app_di.go (3)
44-44: LGTM!The removal of the
UnorderedTxManagerfield indicates a shift in how transactions are managed within the application.The code changes are approved.
44-44: LGTM!The removal of the
Closefunction suggests that the application no longer requires this resource management.The code changes are approved.
44-44: LGTM!The removal of the
GetKeyfunction indicates a broader refactoring of how keys are handled within the application.The code changes are approved.
![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: 1
Outside diff range, codebase verification and nitpick comments (4)
simapp/CHANGELOG.md (4)
35-35: Fix typographical error.Replace "url" with "URL".
- * Update module path to new vanity url. + * Update module path to new vanity URL.
38-38: Fix grammatical issue.Replace "make use of" with "use".
- * Update `export` function to make use of the new module collections API. + * Update `export` function to use the new module collections API.
41-41: Add missing period.Add a period at the end of the sentence.
- * Update testnet command to match new module APIs + * Update testnet command to match new module APIs.
46-46: Fix grammatical issue.Replace "make use of" with "use".
- * [#20490](https://github.com/cosmos/cosmos-sdk/pull/20490) Refactor simulations to make use of `testutil/sims` instead of `runsims`. + * [#20490](https://github.com/cosmos/cosmos-sdk/pull/20490) Refactor simulations to use `testutil/sims` instead of `runsims`.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (8)
- runtime/app.go (5 hunks)
- runtime/module.go (5 hunks)
- simapp/CHANGELOG.md (1 hunks)
- simapp/ante.go (2 hunks)
- simapp/app.go (2 hunks)
- simapp/app_di.go (8 hunks)
- x/auth/ante/ante.go (3 hunks)
- x/auth/tx/config/depinject.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- runtime/app.go
- runtime/module.go
Additional context used
Path-based instructions (6)
simapp/ante.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/ante/ante.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/auth/tx/config/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (12)
simapp/ante.go (3)
Line range hint
7-9: Ensure the import path is correct.The import path for
unorderedtxshould be verified to ensure it is correct and points to the intended package.
Line range hint
27-27: Ensure the new field is initialized properly.The new field
UnorderedTxManagershould be initialized properly when creating an instance ofHandlerOptions.
41-41: Verify the new decorator usage.Ensure that the
NewUnorderedTxDecoratoris correctly used and that the parameters passed to it are valid.x/auth/ante/ante.go (3)
7-7: Ensure the import path is correct.The import path for
unorderedtxshould be verified to ensure it is correct and points to the intended package.
27-27: Ensure the new field is initialized properly.The new field
UnorderedTxManagershould be initialized properly when creating an instance ofHandlerOptions.
58-60: Verify the new decorator usage.Ensure that the
NewUnorderedTxDecoratoris correctly used and that the parameters passed to it are valid.x/auth/tx/config/depinject.go (2)
62-62: LGTM!The addition of the
UnorderedTxManagerfield to theModuleInputsstruct is correctly implemented.The code changes are approved.
167-173: LGTM!The integration of the
UnorderedTxManagerinto theante.NewAnteHandlercall is correctly implemented.The code changes are approved.
simapp/app_di.go (2)
266-266: LGTM!The removal of the
UnorderedTxManagerinitialization and related functionalities is correctly implemented.The code changes are approved.
296-302: LGTM!The removal of the
UnorderedTxManagerfield from theante.HandlerOptionsis correctly implemented.The code changes are approved.
simapp/app.go (2)
641-641: LGTM!The integration of the
UnorderedTxManagerinto theante.HandlerOptionsis correctly implemented.The code changes are approved.
668-671: LGTM!The addition of error handling for the
BaseApp.Close()call is correctly implemented.The code changes are approved.
| if err := manager.RegisterExtensions( | ||
| unorderedtx.NewSnapshotter(app.UnorderedTxManager), | ||
| ) | ||
| if err != nil { | ||
| ); err != nil { |
There was a problem hiding this comment.
Remove unused code.
The unorderedtx.NewSnapshotter(app.UnorderedTxManager) call is no longer necessary and should be removed.
Apply this diff to remove the unused code:
- if err := manager.RegisterExtensions(
- unorderedtx.NewSnapshotter(app.UnorderedTxManager),
- ); err != nil {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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := manager.RegisterExtensions( | |
| unorderedtx.NewSnapshotter(app.UnorderedTxManager), | |
| ) | |
| if err != nil { | |
| ); err != nil { |
There was a problem hiding this comment.
Not true, we haven't abstracted that in runtime
There was a problem hiding this comment.
@julienrbrt, thank you for the clarification. I understand now that the unorderedtx.NewSnapshotter(app.UnorderedTxManager) call is still necessary since it hasn't been abstracted in the runtime. I'll retract my previous suggestion to remove it.
Learnings added
Learnt from: julienrbrt
PR: cosmos/cosmos-sdk#21310
File: simapp/app_di.go:273-275
Timestamp: 2024-08-26T21:27:49.027Z
Learning: When reviewing code related to `unorderedtx.NewSnapshotter(app.UnorderedTxManager)`, remember that it hasn't been abstracted in the runtime, so it should not be removed unless explicitly refactored.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (8)
- UPGRADING.md (2 hunks)
- runtime/app.go (5 hunks)
- simapp/app.go (2 hunks)
- simapp/app_config.go (1 hunks)
- simapp/v2/app_config.go (1 hunks)
- testutil/network/network.go (2 hunks)
- x/auth/ante/ante.go (3 hunks)
- x/auth/tx/config/depinject.go (3 hunks)
Files skipped from review due to trivial changes (1)
- simapp/app_config.go
Files skipped from review as they are similar to previous changes (4)
- simapp/app.go
- simapp/v2/app_config.go
- x/auth/ante/ante.go
- x/auth/tx/config/depinject.go
Additional context used
Path-based instructions (3)
runtime/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testutil/network/network.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.UPGRADING.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (10)
runtime/app.go (3)
45-46: LGTM!The addition of
UnorderedTxManagerin theAppstruct is appropriate for managing unordered transactions.The code changes are approved.
160-172: LGTM!The
Closefunction correctly handles the closure ofUnorderedTxManagerand ensures proper resource management.The code changes are approved.
277-287: LGTM!The
GetKeyfunction is correctly implemented for retrieving theKVStoreKeyfor a specified store key, intended for testing purposes.The code changes are approved.
testutil/network/network.go (1)
214-217: LGTM!The addition of
simtestutil.NewAppOptionsWithFlagHome(val.GetViper().GetString(flags.FlagHome))in thedepinject.Supplyfunction call enhances the configuration capabilities.The code changes are approved.
UPGRADING.md (6)
Line range hint
5-30: LGTM!The section is well-written and provides clear instructions for simulating nested messages.
The documentation is clear and correct.
Line range hint
32-202: LGTM!The section is comprehensive and provides detailed instructions for the changes in the
SimApppackage.The documentation is clear and correct.
Line range hint
204-216: LGTM!The section is well-written and provides clear instructions for the changes in
Depinject app_config.go / app.yml.The documentation is clear and correct.
Line range hint
218-246: LGTM!The section is comprehensive and provides detailed instructions for the changes in the Protobuf package.
The documentation is clear and correct.
Line range hint
248-418: LGTM!The section is comprehensive and provides detailed instructions for the changes in various modules.
The documentation is clear and correct.
Line range hint
420-624: LGTM!The section is well-written and provides clear instructions for the changes in the tooling.
The documentation is clear and correct.
| if err := a.app.ModuleManager.RegisterServices(a.app.configurator); err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| return a.app | ||
| } | ||
|
|
||
| // register unordered tx manager | ||
| func (a *AppBuilder) registerUnorderedTxManager() error { |
(cherry picked from commit 03d3a93) # Conflicts: # runtime/v2/builder.go # runtime/v2/module.go
Description
ref: #20955
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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation