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(runtime,runtime/v2): skip store for specified modules #20409

Merged
merged 5 commits into from
May 27, 2024

Conversation

julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented May 16, 2024

Description

Closes: #20317

The backported PR will contain only the API changes and runtime v0 changes.


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

  • included the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

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

    • Added a SkipStoreKeys field to module configuration to skip specified store keys during configuration.
  • Improvements

    • Enhanced module configuration to handle skipping of store keys for increased flexibility and customization.
  • Bug Fixes

    • Corrected initialization logic for key-value store services to handle modules without stores effectively.
  • Documentation

    • Updated Go module and proto files to include the new skip_store_keys field in module configuration.

@julienrbrt julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label May 16, 2024
@julienrbrt julienrbrt requested a review from a team as a code owner May 16, 2024 12:51
Copy link
Contributor

coderabbitai bot commented May 16, 2024

Warning

Rate Limit Exceeded

@julienrbrt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 49 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits Files that changed from the base of the PR and between bb048aa and 9e715a1.

Walkthrough

The changes involve introducing a new type _Module_11_list for managing lists of strings and adding a SkipStoreKeys field to the Module struct. This field allows specific store keys to be excluded during keeper construction. Various functions have been updated to handle this new field, ensuring that designated store keys are skipped when necessary.

Changes

Files Change Summary
api/cosmos/app/runtime/v1alpha1/module.pulsar.go Added _Module_11_list type and associated methods, and SkipStoreKeys field to Module struct.
go.work.example Removed the toolchain version declaration.
proto/cosmos/app/runtime/v1alpha1/module.proto Added skip_store_keys field to the Module message.
runtime/module.go Modified functions to include config parameters and added conditional checks for SkipStoreKeys.
runtime/store.go Introduced failingStoreService type and methods for handling unavailable KV stores.
runtime/v2/module.go Updated initialization of kvService and memKvService based on SkipStoreKeys.
runtime/v2/stub.go Introduced failingStoreService with methods to handle unavailable KV stores.
simapp/app_config.go Added SkipStoreKeys with "tx" value in module configuration.

Assessment against linked issues

Objective Addressed Explanation
Ensure tx store key is skipped during App v2 initialization to avoid upgrade failure (#20317)
Modify Module struct to include SkipStoreKeys field for specifying store keys to skip (#20317)
Update keeper construction logic to respect SkipStoreKeys field (#20317)

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

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 Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between e034766 and 9acd041.
Files ignored due to path filters (1)
  • api/go.mod is excluded by !**/*.mod
Files selected for processing (10)
  • api/cosmos/app/runtime/v1alpha1/module.pulsar.go (17 hunks)
  • api/cosmos/app/runtime/v2/module.pulsar.go (17 hunks)
  • go.work.example (1 hunks)
  • proto/cosmos/app/runtime/v1alpha1/module.proto (1 hunks)
  • proto/cosmos/app/runtime/v2/module.proto (1 hunks)
  • runtime/module.go (4 hunks)
  • runtime/store.go (1 hunks)
  • runtime/v2/module.go (3 hunks)
  • runtime/v2/stub.go (1 hunks)
  • simapp/app_config.go (1 hunks)
Files not reviewed due to errors (2)
  • api/cosmos/app/runtime/v1alpha1/module.pulsar.go (no review received)
  • api/cosmos/app/runtime/v2/module.pulsar.go (no review received)
Files skipped from review due to trivial changes (1)
  • go.work.example
Additional Context Used
Path-based Instructions (7)
runtime/v2/stub.go (1)

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

runtime/store.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/module.go (1)

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

simapp/app_config.go (1)

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

api/cosmos/app/runtime/v1alpha1/module.pulsar.go (1)

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

api/cosmos/app/runtime/v2/module.pulsar.go (1)

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

Additional comments not posted (22)
runtime/v2/stub.go (3)

11-13: LGTM! The panic message is clear and provides guidance on what to check.


15-17: LGTM! The panic message is clear and provides guidance on what to check.


19-21: LGTM! The panic message is clear and provides guidance on what to check.

proto/cosmos/app/runtime/v2/module.proto (1)

57-60: LGTM! The comments are clear and provide useful information about the skip_store_keys field.

proto/cosmos/app/runtime/v1alpha1/module.proto (1)

41-44: LGTM! The comments are clear and provide useful information about the skip_store_keys field.

runtime/store.go (3)

53-55: LGTM! The panic message is clear and provides guidance on what to check.


57-59: LGTM! The panic message is clear and provides guidance on what to check.


61-63: LGTM! The panic message is clear and provides guidance on what to check.

runtime/v2/module.go (6)

6-6: Import of slices package looks good.


189-191: Initialization of kvService and memKvService with failingStoreService looks good.


194-195: Conditional logic for skipping modules without a store looks good.


204-209: Store key registration and service initialization logic looks good.


225-225: Return statement of ProvideEnvironment function looks good.


204-204: Store key registration logic in registerStoreKey function looks good.

runtime/module.go (7)

6-6: Import of slices package looks good.


196-197: Conditional logic for skipping store keys in ProvideKVStoreKey function looks good.


199-199: Store key registration logic in ProvideKVStoreKey function looks good.


219-220: Conditional logic for skipping transient store keys in ProvideTransientStoreKey function looks good.


222-222: Transient store key registration logic in ProvideTransientStoreKey function looks good.


233-234: Conditional logic for skipping memory store keys in ProvideMemoryStoreKey function looks good.


236-236: Memory store key registration logic in ProvideMemoryStoreKey function looks good.

simapp/app_config.go (1)

173-177: LGTM! Ensure the SkipStoreKeys field is correctly used and documented.

Verification successful

The SkipStoreKeys field is used in several files across the codebase. To ensure it is correctly used and documented, we need to verify its usage in the following files:

  1. simapp/app_config.go
  2. runtime/module.go
  3. runtime/v2/module.go
  4. api/cosmos/app/runtime/v2/module.pulsar.go
  5. api/cosmos/app/runtime/v1alpha1/module.pulsar.go

Let's start by examining the context and usage of SkipStoreKeys in these files to ensure it is implemented correctly.

Verification Steps:

  1. Check simapp/app_config.go:

    • Ensure SkipStoreKeys is defined and used appropriately.
    • Verify any related comments or documentation.
  2. Check runtime/module.go:

    • Verify the logic involving SkipStoreKeys in the slices.Contains checks.
    • Ensure the context of these checks makes sense.
  3. Check runtime/v2/module.go:

    • Similar to runtime/module.go, verify the logic and context of SkipStoreKeys.
  4. Check api/cosmos/app/runtime/v2/module.pulsar.go:

    • Ensure SkipStoreKeys is used correctly in the protobuf definitions and related logic.
    • Verify any related comments or documentation.
  5. Check api/cosmos/app/runtime/v1alpha1/module.pulsar.go:

    • Similar to api/cosmos/app/runtime/v2/module.pulsar.go, verify the usage and documentation.

To gather more detailed information, let's generate shell scripts to extract the relevant code snippets from these files.


Analysis of SkipStoreKeys Usage:

  1. simapp/app_config.go:

    • SkipStoreKeys is defined as an optional list of store keys to skip when constructing the module's keeper.
    • The comment explains its purpose clearly.
  2. runtime/module.go:

    • SkipStoreKeys is used in conditional checks to determine if a store key should be skipped.
    • The logic appears consistent and correct.
  3. runtime/v2/module.go:

    • Similar usage as in runtime/module.go, with conditional checks to skip store keys.
    • The implementation is consistent with the intended functionality.
  4. api/cosmos/app/runtime/v2/module.pulsar.go:

    • SkipStoreKeys is used in protobuf definitions and related logic.
    • The field is documented in the protobuf comments, explaining its purpose.
  5. api/cosmos/app/runtime/v1alpha1/module.pulsar.go:

    • Similar usage as in api/cosmos/app/runtime/v2/module.pulsar.go, with protobuf definitions and logic.
    • The field is documented in the protobuf comments.

Conclusion:

The SkipStoreKeys field is correctly used and documented across the codebase. The implementation aligns with the intended functionality, and the comments provide clear explanations of its purpose.

Final Response:

Several files in the codebase correctly use and document the SkipStoreKeys field.

  • simapp/app_config.go
  • runtime/module.go
  • runtime/v2/module.go
  • api/cosmos/app/runtime/v2/module.pulsar.go
  • api/cosmos/app/runtime/v1alpha1/module.pulsar.go
Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the usage of the `SkipStoreKeys` field in the codebase.

# Test: Search for the usage of the `SkipStoreKeys` field. Expect: Correct usage in relevant files.
rg --type go 'SkipStoreKeys'

Length of output: 5299



Script:

#!/bin/bash
# Extract relevant code snippets for SkipStoreKeys usage in the specified files.

# Extract from simapp/app_config.go
echo "simapp/app_config.go:"
rg --context 5 'SkipStoreKeys' simapp/app_config.go

# Extract from runtime/module.go
echo -e "\nruntime/module.go:"
rg --context 5 'SkipStoreKeys' runtime/module.go

# Extract from runtime/v2/module.go
echo -e "\nruntime/v2/module.go:"
rg --context 5 'SkipStoreKeys' runtime/v2/module.go

# Extract from api/cosmos/app/runtime/v2/module.pulsar.go
echo -e "\napi/cosmos/app/runtime/v2/module.pulsar.go:"
rg --context 5 'SkipStoreKeys' api/cosmos/app/runtime/v2/module.pulsar.go

# Extract from api/cosmos/app/runtime/v1alpha1/module.pulsar.go
echo -e "\napi/cosmos/app/runtime/v1alpha1/module.pulsar.go:"
rg --context 5 'SkipStoreKeys' api/cosmos/app/runtime/v1alpha1/module.pulsar.go

Length of output: 15091

runtime/store.go Dismissed Show dismissed Hide dismissed
runtime/v2/stub.go Dismissed Show dismissed Hide dismissed
@kocubinski
Copy link
Member

I don't follow #20317 or this PR to fix it. if a module doesn't use store, how is a store key being registered? that will only happen if the module requests one as a input for depinject. furthermore, x/tx ("tx") has no depinject configuration, so how this happening?

@julienrbrt
Copy link
Member Author

You are going to hate it, but it isn't x/tx but x/auth/tx (https://github.com/cosmos/cosmos-sdk/blob/main/x/auth/tx/config/depinject.go)

@julienrbrt
Copy link
Member Author

julienrbrt commented May 16, 2024

I don't follow #20317 or this PR to fix it. if a module doesn't use store, how is a store key being registered? that will only happen if the module requests one as a input for depinject. furthermore, x/tx ("tx") has no depinject configuration, so how this happening?

Yeah and each module require an environment, which has a store service directly included.

I am not too sure about v0.50 to be honest, I haven't tried there. Might as well just been a regression due to the environment introduction.

@julienrbrt julienrbrt removed the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label May 16, 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.

could you add a changelog, a small entry in upgrading and update the docs in depinject to document this feature

@julienrbrt
Copy link
Member Author

added

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: 3

Outside diff range and nitpick comments (43)
simapp/CHANGELOG.md (1)

Line range hint 51-51: Remove extra blank line to adhere to formatting standards.

-
UPGRADING.md (42)

Line range hint 171-171: Change "depending" to "depend" for grammatical correctness.

- If you were depending on `cosmossdk.io/api/tendermint`, please use the buf generated go SDK instead, or ask CometBFT host the generated proto v2 code.
+ If you depend on `cosmossdk.io/api/tendermint`, please use the buf generated go SDK instead, or ask CometBFT to host the generated proto v2 code.

Line range hint 181-181: Change "recommend to use" to "recommend using" for grammatical correctness.

- we strongly recommend to use the `cosmossdk.io/core/appmodule` interfaces for the modules.
+ we strongly recommend using the `cosmossdk.io/core/appmodule` interfaces for the modules.

Line range hint 231-231: Change "prior to" to "before" for conciseness.

- It is required to migrate to v0.50 prior to upgrading to v0.51 for not missing any module migrations.
+ It is required to migrate to v0.50 before upgrading to v0.51 to ensure no module migrations are missed.

Line range hint 252-252: Change "this changes" to "these changes" for grammatical agreement.

- Many functions have been removed due to this changes as the API can be smaller thanks to collections.
+ Many functions have been removed due to these changes as the API can be smaller thanks to collections.

Line range hint 264-264: Add a missing article "its" before "own".

- Bank was spun out into its own `go.mod`.
+ Bank was spun out into its own `go.mod`.

Line range hint 300-300: Add a comma before "and" to correctly connect two independent clauses.

- A standalone Go module was created and it is accessible at "cosmossdk.io/x/params".
+ A standalone Go module was created, and it is accessible at "cosmossdk.io/x/params".

Line range hint 335-335: Add a comma after "versions" for clarity.

- The Cosmos SDK has migrated in its previous versions, to CometBFT.
+ The Cosmos SDK has migrated in its previous versions, to CometBFT.

Line range hint 368-368: Add a comma after "chains" for clarity.

- For existing chains this can be done in two ways:
+ For existing chains, this can be done in two ways:

Line range hint 370-370: Add a comma after "upgrade" for clarity.

- During an upgrade the value is set in an upgrade handler.
+ During an upgrade, the value is set in an upgrade handler.

Line range hint 377-377: Add a comma before "and" to correctly connect two independent clauses.

- where a catastrophic failure has occurred and the application should halt.
+ where a catastrophic failure has occurred, and the application should halt.

Line range hint 379-379: Change "by" to "be" for grammatical correctness.

- will gracefully by handled by `BaseApp` on behalf of the application.
+ will gracefully be handled by `BaseApp` on behalf of the application.

Line range hint 384-384: Simplify "in order to" to "to" for conciseness.

- `FinalizeBlock` is public and should be used in order to test and run operations.
+ `FinalizeBlock` is public and should be used to test and run operations.

Line range hint 397-397: Change "allows to modify" to "allows modification of" for grammatical correctness.

- and allows to modify consensus parameters, and the changes are visible to the following state machine logics.
+ and allows modification of consensus parameters, and the changes are visible to the following state machine logics.

Line range hint 435-435: Add a comma after "Instead" for clarity.

- Instead a new attribute is added to all messages indicating the `msg_index` which identifies which events and attributes relate the same transaction.
+ Instead, a new attribute is added to all messages indicating the `msg_index` which identifies which events and attributes relate the same transaction.

Line range hint 462-462: Change "a" to "an" before "unsupported".

- To migrate from a unsupported database to a supported database please use a database migration tool.
+ To migrate from an unsupported database to a supported database please use a database migration tool.

Line range hint 466-466: Add a comma after "database" for clarity.

- To migrate from an unsupported database to a supported database please use a database migration tool.
+ To migrate from an unsupported database to a supported database, please use a database migration tool.

Line range hint 483-483: Add a missing apostrophe for "SDK's".

- In this section we describe the changes made in Cosmos SDK' SimApp.
+ In this section we describe the changes made in Cosmos SDK's SimApp.

Line range hint 547-547: Add a comma before "and" to correctly connect two independent clauses.

- The global variable has been removed and the basic module manager can be now created from the module manager.
+ The global variable has been removed, and the basic module manager can now be created from the module manager.

Line range hint 583-583: Add a comma after "Additionally" for clarity.

- Additionally `AutoCLI` automatically adds the custom modules commands to the root command for all modules implementing the [`appmodule.AppModule`](https://pkg.go.dev/cosmossdk.io/core/appmodule#AppModule) interface.
+ Additionally, `AutoCLI` automatically adds the custom modules commands to the root command for all modules implementing the [`appmodule.AppModule`](https://pkg.go.dev/cosmossdk.io/core/appmodule#AppModule) interface.

Line range hint 614-614: Add a missing preposition "to" for grammatical correctness.

- which allows it be a standalone module.
+ which allows it to be a standalone module.

Line range hint 635-635: Change "human readable" to "human-readable" for grammatical correctness.

- that produces more human readable output, currently only available on Ledger devices but soon to be implemented in other UIs.
+ that produces more human-readable output, currently only available on Ledger devices but soon to be implemented in other UIs.

Line range hint 679-679: Remove the extraneous "the" for grammatical correctness.

- When using `depinject` / `app v2`, the a tx config should be recreated from the `txConfigOpts` to use `NewGRPCCoinMetadataQueryFn` instead of depending on the bank keeper (that is used in the server).
+ When using `depinject` / `app v2`, a tx config should be recreated from the `txConfigOpts` to use `NewGRPCCoinMetadataQueryFn` instead of depending on the bank keeper (that is used in the server).

Line range hint 691-691: Change "implementations of" to "implementation of" for grammatical correctness.

- and implementations of `GetSignBytes` can be deleted.
+ and implementation of `GetSignBytes` can be deleted.

Line range hint 695-695: Change "an interfaces" to "interfaces" for grammatical correctness.

- Any module that has an interfaces for them (like "expected keepers") will need to update and re-generate mocks if needed:
+ Any module that has interfaces for them (like "expected keepers") will need to update and re-generate mocks if needed:

Line range hint 719-719: Add a comma after "case" for clarity.

- In case a module requires to return `abci.ValidatorUpdate` from `EndBlock`, it can use the `HasABCIEndBlock` interface instead.
+ In case, a module requires to return `abci.ValidatorUpdate` from `EndBlock`, it can use the `HasABCIEndBlock` interface instead.

Line range hint 746-746: Add a comma after "function" for clarity.

- To find out more please read the [signer field](https://github.com/cosmos/cosmos-sdk/blob/main/docs/build/building-modules/05-protobuf-annotations.md) & [here](https://github.com/cosmos/cosmos-sdk/blob/7352d0bce8e72121e824297df453eb1059c28da8/docs/docs/build/building-modules/02-messages-and-queries.md#L40) documentation.
+ To find out more, please read the [signer field](https://github.com/cosmos/cosmos-sdk/blob/main/docs/build/building-modules/05-protobuf-annotations.md) & [here](https://github.com/cosmos/cosmos-sdk/blob/7352d0bce8e72121e824297df453eb1059c28da8/docs/docs/build/building-modules/02-messages-and-queries.md#L40) documentation.

Line range hint 761-761: Change "a" to "an" before "application".

- The Cosmos SDK has migrated from a CometBFT genesis to a application managed genesis file.
+ The Cosmos SDK has migrated from a CometBFT genesis to an application managed genesis file.

Line range hint 781-781: Change "an higher" to "a higher".

- An expedited proposal must have an higher voting threshold than a classic proposal, that threshold is defined with the `ExpeditedThreshold` parameter.
+ An expedited proposal must have a higher voting threshold than a classic proposal, that threshold is defined with the `ExpeditedThreshold` parameter.

Line range hint 798-798: Add a missing preposition "to" for grammatical correctness.

- which allows it be a standalone module.
+ which allows it to be a standalone module.

Line range hint 805-805: Add a comma after "file" for clarity.

- which allows it to be a standalone module.
+ which allows it to be a standalone module,

Line range hint 819-819: Add a comma after "file" for clarity.

- which allows it to be a standalone module.
+ which allows it to be a standalone module,

Line range hint 826-826: Change "it's" to "its" for possessive form.

- Rosetta has moved to it's own [repo](https://github.com/cosmos/rosetta) and not imported by the Cosmos SDK SimApp by default.
+ Rosetta has moved to its own [repo](https://github.com/cosmos/rosetta) and not imported by the Cosmos SDK SimApp by default.

Line range hint 827-827: Change "on" to "in" for correct preposition usage.

- Any user who is interested on using the tool can connect it standalone to any node without the need to add it as part of the node binary.
+ Any user who is interested in using the tool can connect it standalone to any node without the need to add it as part of the node binary.

Line range hint 828-828: Change "multi chain" to "multi-chain" for correct compound adjective usage.

- The rosetta tool also allows multi chain connections.
+ The rosetta tool also allows multi-chain connections.

Line range hint 850-850: Change "however" to "but" for correct conjunction usage.

- Remove `RandomizedParams` from `AppModuleSimulation` interface. Previously, it used to generate random parameter changes during simulations, however, it does so through ParamChangeProposal which is now legacy.
+ Remove `RandomizedParams` from `AppModuleSimulation` interface. Previously, it used to generate random parameter changes during simulations, but it does so through ParamChangeProposal which is now legacy.

Line range hint 852-852: Change "modules" to "module" for singular noun agreement.

- To support the `MsgUpdateParams` governance proposals for each modules, `AppModuleSimulation` now defines a `AppModule.ProposalMsgs` method in addition to `AppModule.ProposalContents`.
+ To support the `MsgUpdateParams` governance proposals for each module, `AppModuleSimulation` now defines an `AppModule.ProposalMsgs` method in addition to `AppModule.ProposalContents`.

Line range hint 867-867: Change "previously" to "formerly" for formal tone.

- Previously, all modules were required to be set in `OrderBeginBlockers`, `OrderEndBlockers` and `OrderInitGenesis / OrderExportGenesis` in `app.go` / `app_config.go`.
+ Formerly, all modules were required to be set in `OrderBeginBlockers`, `OrderEndBlockers` and `OrderInitGenesis / OrderExportGenesis` in `app.go` / `app_config.go`.

Line range hint 884-884: Simplify "in order to" to "to" for conciseness.

- add the following lines to your `app.go` in order to provide newer gRPC services:
+ add the following lines to your `app.go` to provide newer gRPC services:

Line range hint 901-901: Add a comma after "arguments" for clarity.

- These were unnecessary given as arguments as they were already present in the `AppOptions`.
+ These were unnecessary, given as arguments, as they were already present in the `AppOptions`.

Line range hint 905-905: Add a comma after "Instead" for clarity.

- Instead you can use the `TestEncodingConfig` from the `types/module/testutil` package.
+ Instead, you can use the `TestEncodingConfig` from the `types/module/testutil` package.

Line range hint 915-915: Change "must pinned" to "must be pinned".

- The `GoLevelDB` version must pinned to `v1.0.1-0.20210819022825-2ae1ddf74ef7` in the application, following versions might cause unexpected behavior.
+ The `GoLevelDB` version must be pinned to `v1.0.1-0.20210819022825-2ae1ddf74ef7` in the application, following versions might cause unexpected behavior.

Line range hint 926-926: Change "replace" to "replacement".

- This allows you to remove the replace directive `replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1` from your `go.mod` file.
+ This allows you to remove the replacement directive `replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1` from your `go.mod` file.
Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 9acd041 and bb048aa.
Files selected for processing (2)
  • UPGRADING.md (3 hunks)
  • simapp/CHANGELOG.md (1 hunks)
Additional Context Used
LanguageTool (71)
UPGRADING.md (70)

Near line 10: It appears that a comma is missing.
Context: .... ## [Unreleased] ### SimApp In this section we describe the changes made in Cosmos ...


Near line 10: Unpaired symbol: ‘'’ seems to be missing
Context: ... describe the changes made in Cosmos SDK' SimApp. **These changes are directly ap...


Near line 171: The verb ‘depend’ can be stative. If ‘depending’ describes a state, change the sentence structure and use the base form of the verb.
Context: ...ild/docs/bsr/generated-sdks/go). If you were depending on cosmossdk.io/api/tendermint, pleas...


Near line 181: The verb ‘recommend’ is used with the gerund form.
Context: ...precation of sdk.Context, we strongly recommend to use the cosmossdk.io/core/appmodule inter...


Near line 225: Possible missing comma found.
Context: ... (sdk.Msg, error) ``` ##### Depinject Previously cosmossdk.io/core held functions `Inv...


Near line 231: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...ved. It is required to migrate to v0.50 prior to upgrading to v0.51 for not missing any ...


Near line 252: The singular determiner ‘this’ may not agree with the plural noun ‘changes’. Did you mean “these”?
Context: ...Many functions have been removed due to this changes as the API can be smaller thank...


Near line 264: Possible missing article found.
Context: ... cosmossdk.io/x/authz #### x/bank Bank was spun out into its own go.mod. To ...


Near line 300: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ams` A standalone Go module was created and it is accessible at "cosmossdk.io/x/par...


Near line 335: Possible missing comma found.
Context: ...o CometBFT (Part 2) The Cosmos SDK has migrated in its previous versions, to CometBFT. ...


Near line 368: It appears that a comma is missing.
Context: ...genesis.json` file. For existing chains this can be done in two ways: * During an u...


Near line 370: Consider adding a comma after this introductory phrase.
Context: ...s can be done in two ways: * During an upgrade the value is set in an upgrade handler....


Near line 377: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...here a catastrophic failure has occurred and the application should halt. However, t...


Near line 379: Did you maybe mean “buy” or “be”?
Context: ... that returns an error, will gracefully by handled by BaseApp on behalf of the a...


Near line 384: Consider a shorter alternative to avoid wordiness.
Context: ...lizeBlock` is public and should be used in order to test and run operations. This means tha...


Near line 397: Did you mean “modifying”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...begin blocker other modules, and allows to modify consensus parameters, and the changes a...


Near line 435: A comma may be missing after the conjunctive/linking adverb ‘Instead’.
Context: ...he case of successful msg(s) execution. Instead a new attribute is added to all message...


Near line 462: The word “clean-up” is a noun. The verb is spelled with a white space.
Context: ...s well as its settings. Use confix to clean-up your app.toml. A nginx (or alike) rev...


Near line 466: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... not supported anymore. To migrate from a unsupported database to a supported dat...


Near line 466: Consider adding a comma here.
Context: ...pported database to a supported database please use a database migration tool. ### Pro...


Near line 483: It appears that a comma is missing.
Context: ...tate-machine code. ### SimApp In this section we describe the changes made in Cosmos ...


Near line 483: Unpaired symbol: ‘'’ seems to be missing
Context: ... describe the changes made in Cosmos SDK' SimApp. **These changes are directly ap...


Near line 547: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...on. The global variable has been removed and the basic module manager can be now cre...


Near line 583: A comma may be missing after the conjunctive/linking adverb ‘Additionally’.
Context: ... not to be included in the binary. ::: Additionally AutoCLI automatically adds the custom...


Near line 614: Possible missing comma found.
Context: ... is extracted to have a separate go.mod file which allows it be a standalone module....


Near line 614: The preposition ‘to’ may be missing (allow someone to do something).
Context: ... a separate go.mod file which allows it be a standalone module. All the store impo...


Near line 635: This word is normally spelled with a hyphen.
Context: ...available in the SDK that produces more human readable output, currently only available on Led...


Near line 679: Two determiners in a row. Choose either “the” or “a”.
Context: ...``` When using depinject / `app v2`, the a tx config should be recreated from the ...


Near line 691: Possible missing comma found.
Context: ...onger need to implement the LegacyMsg interface and implementations of GetSignBytes c...


Near line 695: It looks like ‘interfaces’ doesn’t match ‘an’. Did you mean “an interface” or just “interfaces”?
Context: ...d of sdk.Context. Any module that has an interfaces for them (like "expected keepers") will...


Near line 719: Possible missing comma found.
Context: ...EndBlock(context.Context) error ``` In case a module requires to return `abci.Valid...


Near line 746: Possible missing comma found.
Context: ...ge. The signer field is required on all messages unless using a custom signer function. ...


Near line 748: Consider adding a comma here.
Context: ...ustom signer function. To find out more please read the [signer field](https://github....


Near line 761: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...has migrated from a CometBFT genesis to a application managed genesis file. The g...


Near line 781: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...ameter. An expedited proposal must have an higher voting threshold than a classic ...


Near line 798: The preposition ‘to’ may be missing (allow someone to do something).
Context: ... a separate go.mod file which allows it be a standalone module. All the evidence i...


Near line 805: Possible missing comma found.
Context: ... is extracted to have a separate go.mod file which allows it to be a standalone modu...


Near line 819: Possible missing comma found.
Context: ... is extracted to have a separate go.mod file which allows it to be a standalone modu...


Near line 826: Did you mean the possessive pronoun “its”?
Context: ...ing #### Rosetta Rosetta has moved to it's own [repo](https://github.com/cosmos/ro...


Near line 827: After the verb ‘interested’ the preposition “in” is used.
Context: ... by default. Any user who is interested on using the tool can connect it standalon...


Near line 828: This word is normally spelled as one.
Context: ...de binary. The rosetta tool also allows multi chain connections. ## [v0.47.x](https://gith...


Near line 850: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...dom parameter changes during simulations, however, it does so through ParamChangeProposal ...


Near line 852: The noun should probably be in the singular form.
Context: ...teParamsgovernance proposals for each modules,AppModuleSimulationnow defines aA...


Near line 867: Consider using “formerly” to strengthen your wording.
Context: ... modules no longer support the REST API previously known as the LCD, and the `sdk.Msg#Rout...


Near line 884: Consider a shorter alternative to avoid wordiness.
Context: ...dd the following lines to your app.go in order to provide newer gRPC services: ```go aut...


Near line 901: Possible missing comma found.
Context: ...riod). These were unnecessary given as arguments as they were already present in the Ap...


Near line 905: A comma may be missing after the conjunctive/linking adverb ‘Instead’.
Context: ...)was deprecated and has been removed. Instead you can use theTestEncodingConfig` fr...


Near line 915: The modal verb ‘must’ requires the verb’s base form.
Context: ... Replaces The GoLevelDB version must pinned to `v1.0.1-0.20210819022825-2ae1ddf74ef...


Near line 926: The word ‘replace’ is a verb. Did you mean the noun “replacement”?
Context: ...goproto. This allows you to remove the replace directive replace github.com/gogo/prot...


Near line 934: You have used ‘I’ with an apostrophe, but either the verb is missing or you have not used the corresponding abbreviated form. Consider using one of the suggestions or removing the apostrophe.
Context: ...ly the root proto/ folder, set by the protoc -I flag). For example, assuming you put a...


Near line 944: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ace` annotations. We require them to be fully-scoped names. They will soon be used by code g...


Near line 962: The word “also” tends to be overused. Consider using a formal alternative to strengthen your wording.
Context: ...os.feegrant.v1beta1.FeeAllowanceI" ``` Please also check that in your own app's proto file...


Near line 963: Try using a more formal synonym for ‘check’.
Context: ...v1beta1.FeeAllowanceI" ``` Please also check that in your own app's proto files that...


Near line 963: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...otations. If so, then replace them with fully-qualified names, even though those names don't ac...


Near line 1003: In American English, “take a look” is more commonly used.
Context: ...e modules keepers in previous versions. Have a look at simapp.RegisterUpgradeHandlers() f...


Near line 1021: Did you mean “At a Time”, “At the Time”, or “At times”?
Context: ...x/gov ##### Minimum Proposal Deposit At Time of Submission The gov module has bee...


Near line 1028: Did you mean “at a time”, “at the time”, or “at times”?
Context: ...o utilize the minimum proposal deposits at time of submission, the migration logic need...


Near line 1060: A comma is probably missing here.
Context: ...ng Tendermint consensus parameters. For migration it is required to call a specific migra...


Near line 1087: Consider a shorter alternative to avoid wordiness.
Context: ...should still be imported in your app.go in order to handle this migration. Because the `x/...


Near line 1149: Consider a shorter alternative to avoid wordiness.
Context: ...ally, new packages have been introduced in order to further split the codebase. Aliases are...


Near line 1181: Possible missing comma found.
Context: ...ndex represents the latest state of the IAVL laid out in a format that preserves dat...


Near line 1201: It appears that a comma is missing.
Context: ...aintained until April 18, 2023. At this point the module will reach end of life and b...


Near line 1206: Consider a shorter alternative to avoid wordiness.
Context: ...the new implementation is called v1. In order to submit a proposal with `submit-proposal...


Near line 1213: ‘by accident’ might be wordy. Consider a shorter alternative.
Context: ...g delegations. Users that have unbonded by accident or wish to cancel a undelegation can no...


Near line 1213: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... unbonded by accident or wish to cancel a undelegation can now specify the amount...


Near line 1217: The auxiliary verb ‘do’ requires the base form of the verb.
Context: ...v0.45.3/third_party/proto) now does not contains directly the [proto files](https://gith...


Near line 1219: Consider a shorter alternative to avoid wordiness.
Context: ...build/cosmos/cosmos-sdk` as dependency, in order to avoid having to copy paste these files....


Near line 1219: Did you mean “copy and paste”?
Context: ...dependency, in order to avoid having to copy paste these files. The protos can as well be...


Near line 1237: Possible missing comma found.
Context: ..."]; } ``` When clients interact with a node they are required to set a codec in in ...


Near line 1237: Possible typo: you repeated a word
Context: ...a node they are required to set a codec in in the grpc.Dial. More information can be ...

simapp/CHANGELOG.md (1)

Near line 28: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...application. This changelog is aimed to help developers understand the wiring change...

Path-based Instructions (2)
simapp/CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

UPGRADING.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

simapp/CHANGELOG.md Show resolved Hide resolved
UPGRADING.md Outdated Show resolved Hide resolved
@julienrbrt julienrbrt added this pull request to the merge queue May 27, 2024
Merged via the queue into main with commit 190b20c May 27, 2024
67 checks passed
@julienrbrt julienrbrt deleted the julien/runtime-skip-store branch May 27, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: App v2 sets "tx" as store key and this fails on the chain upgrade
4 participants