feat: introduce x/validate and split x/auth/tx/config #21822
Conversation
WalkthroughWalkthroughThe changes in this pull request introduce a new Changes
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (5)
Files skipped from review as they are similar to previous changes (2)
Additional context usedPath-based instructions (3)
Additional comments not posted (8)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
julienrbrt
changed the title
github-actions
bot
added
C:x/auth
Type: CI
C:x/tx
C:x/accounts
C:x/accounts/base
labels
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| protoiface "google.golang.org/protobuf/runtime/protoiface" | ||
| protoimpl "google.golang.org/protobuf/runtime/protoimpl" | ||
| io "io" | ||
| reflect "reflect" |
Check notice
Code scanning / CodeQL
Sensitive package import Note
| switch fd.FullName() { | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.validate.module.v1.Module")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.validate.module.v1.Module")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.validate.module.v1.Module does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
julienrbrt
added
the
backport/v0.52.x
julienrbrt
requested review from
auricom,
tac0turtle,
kocubinski,
testinginprod,
aaronc,
a team,
facundomedica and
sontrinh16
as code owners
|
We can maybe move x/auth/ante to x/validate eventually, but it isn't really necessary right now and for 0.52 (this limits the amount of breakage anyway) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (14)
x/validate/README.md (1)
4-4: Consider adding a comma before "and" (optional).According to the grammar rule, a comma should be used before "and" when it connects two independent clauses, unless they are closely connected and short. In this case, the clauses are closely connected and relatively short, so the comma is optional. However, adding the comma may improve readability:
-This module is only required when using runtime and runtime v2 and you want to make use of the pre-defined ante/poste handlers or tx validators. +This module is only required when using runtime and runtime v2, and you want to make use of the pre-defined ante/poste handlers or tx validators.Tools
LanguageTool
[uncategorized] ~4-~4: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...quired when using runtime and runtime v2 and you want to make use of the pre-defined...(COMMA_COMPOUND_SENTENCE)
x/auth/tx/config/depinject.go (3)
54-54: Clarify the Comment forBaseAppOptionThe comment for
BaseAppOptioncan be made more concise and aligned with GoDoc standards for better readability.Consider rephrasing the comment:
- BaseAppOption runtime.BaseAppOption // This is only useful for chains using baseapp. + BaseAppOption runtime.BaseAppOption // Useful only for chains using baseapp.
99-99: Align Function Comment with GoDoc ConventionsThe comment for
NewBankKeeperCoinMetadataQueryFnshould start with the function name and clearly describe its purpose, following GoDoc guidelines.Update the comment to:
- // NewBankKeeperCoinMetadataQueryFn creates a new Textual struct using the given BankKeeper to retrieve coin metadata. + // NewBankKeeperCoinMetadataQueryFn creates a new `CoinMetadataQueryFn` using the provided `BankKeeper` to retrieve coin metadata.
41-48: Consider Grouping Related DependenciesFor improved readability and consistency with the Uber Golang style guide, consider grouping related codec dependencies together in the
ModuleInputsstruct.Arrange the fields as follows:
Config *txconfigv1.Config -AddressCodec address.Codec -ValidatorAddressCodec address.ValidatorAddressCodec Codec codec.Codec ProtoFileResolver txsigning.ProtoFileResolver +AddressCodec address.Codec +ValidatorAddressCodec address.ValidatorAddressCodec Environment appmodulev2.Environment CustomSignModeHandlers func() []txsigning.SignModeHandler `optional:"true"` CustomGetSigners []txsigning.CustomGetSigner `optional:"true"`x/validate/depinject.go (2)
3-22: Organize import statements according to Go conventionsThe import statements should be organized into three groups separated by blank lines:
- Standard library packages
- Third-party packages
- Local packages
This improves readability and maintains conformity with the Uber Go Style Guide.
Example:
import ( "errors" "fmt" modulev1 "cosmossdk.io/api/cosmos/validate/module/v1" appmodulev2 "cosmossdk.io/core/appmodule/v2" "cosmossdk.io/core/server" "cosmossdk.io/core/transaction" "cosmossdk.io/depinject" "cosmossdk.io/depinject/appconfig" "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/runtime" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/auth/ante" "github.com/cosmos/cosmos-sdk/x/auth/ante/unorderedtx" "github.com/cosmos/cosmos-sdk/x/auth/posthandler" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" )
59-59: Use descriptive variable namesThe variable
svdis not immediately descriptive. Consider renaming it tosigVerificationDecoratorto enhance code readability.Apply this diff:
-svd := ante.NewSigVerificationDecorator( +sigVerificationDecorator := ante.NewSigVerificationDecorator( in.AccountKeeper, in.TxConfig.SignModeHandler(), ante.DefaultSigVerificationGasConsumer, in.AccountAbstractionKeeper, )And update its usage:
-return ModuleOutputs{ - Module: NewAppModule(svd, feeTxValidator, unorderedTxValidator, in.ExtraTxValidators...), +return ModuleOutputs{ + Module: NewAppModule(sigVerificationDecorator, feeTxValidator, unorderedTxValidator, in.ExtraTxValidators...),UPGRADING.md (6)
109-109: Correct capitalization of "gRPC-Web"In the sentence, "Grpc-web embedded client has been removed from the server.", "Grpc-web" should be capitalized as "gRPC-Web" to match the standard naming convention.
Apply this diff to fix the capitalization:
-Grpc-web embedded client has been removed from the server. If you would like to use grpc-web, you can use the [envoy proxy](https://www.envoyproxy.io/docs/envoy/latest/start/start). Here's how to set it up: +gRPC-Web embedded client has been removed from the server. If you would like to use gRPC-Web, you can use the [Envoy proxy](https://www.envoyproxy.io/docs/envoy/latest/start/start). Here's how to set it up:
324-324: Use "no longer" instead of "no more"In the sentence, "sign mode textual is no more automatically configured...", "no more" should be replaced with "no longer" for correct usage.
Apply this diff to correct the phrasing:
-With the split of `x/auth/tx/config` in two (x/auth/tx/config as depinject module for txconfig and tx options) and `x/validate`, sign mode textual is no more automatically configured when using runtime (it was previously the case). +With the split of `x/auth/tx/config` into two (x/auth/tx/config as a depinject module for txconfig and tx options) and `x/validate`, sign mode textual is no longer automatically configured when using runtime (it was previously the case).
325-325: Correct comparative conjunction in the sentenceThe phrase "For the same instructions than for legacy app wiring..." should be corrected to "For the same instructions as for legacy app wiring..." to use the correct comparative conjunction.
Apply this diff to fix the grammatical error:
-For the same instructions than for legacy app wiring to enable sign mode textual (see in v0.50 UPGRADING documentation). +Refer to the same instructions as for legacy app wiring to enable sign mode textual (see in v0.50 UPGRADING documentation).Tools
LanguageTool
[grammar] ~325-~325: Comparison is written ‘the same … as’.
Context: ...ly the case). For the same instructions than for legacy app wiring to enable sign mo...(THE_SAME_AS)
467-467: Improve sentence clarityThe sentence can be rephrased for better readability: "The
x/crisismodule has been removed because it is no longer supported or functional."Apply this diff to improve readability:
-The `x/crisis` module was removed due to it not being supported or functional any longer. +The `x/crisis` module has been removed because it is no longer supported or functional.
513-513: Add comma after "x/validate" for clarityIn the sentence "Introducing
x/validatea module that is solely used...", a comma should be added after "x/validate" to separate the introductory phrase.Apply this diff to improve clarity:
-Introducing `x/validate` a module that is solely used for registering default ante/post handlers and global transaction validators when using runtime and runtime/v2. +Introducing `x/validate`, a module that is solely used for registering default ante/post handlers and global transaction validators when using runtime and runtime/v2.
514-514: Add missing comma after "however"In the sentence "You can however always extend them by adding extra tx validators...", a comma should be added after "however" to improve readability.
Apply this diff to fix the missing comma:
-You can however always extend them by adding extra tx validators (see `x/validate` documentation). +You can, however, always extend them by adding extra tx validators (see `x/validate` documentation).Tools
LanguageTool
[uncategorized] ~514-~514: Possible missing comma found.
Context: ...rs, no need to use this module. You can however always extend them by adding extra tx v...(AI_HYDRA_LEO_MISSING_COMMA)
CHANGELOG.md (2)
50-50: Improve clarity and grammar in the explanatory noteThe sentence on line 50 is grammatically incorrect and could be rephrased for better readability.
Apply this diff to improve clarity:
- * In comparison to x/auth/tx/config, there is no app config to skip ante/post handlers, as overwriting them is baseapp or not injecting the x/validate module has the same effect. + * In comparison to x/auth/tx/config, there is no app config to skip ante/post handlers, because overwriting them in baseapp or not injecting the x/validate module has the same effect.
66-67: Correct grammatical errors and improve clarity in documentationThere are grammatical issues on lines 66-67 that can be corrected for better readability.
Apply the following changes to improve clarity:
-* (x/auth/tx/config) [#21822](https://github.com/cosmos/cosmos-sdk/pull/21822) Sign mode textual is no more automatically added to tx config when using runtime. Should be added manually on the server side. +* (x/auth/tx/config) [#21822](https://github.com/cosmos/cosmos-sdk/pull/21822) Sign mode textual is no longer automatically added to tx config when using runtime. It should be added manually on the server side. -* (x/auth/tx/config) [#21822](https://github.com/cosmos/cosmos-sdk/pull/21822) This depinject module now only provide txconfig and tx config options. `x/validate` now handles the providing of ante/post handlers, alongside tx validators for v2. The corresponding app config options have been removed from the depinject module config. +* (x/auth/tx/config) [#21822](https://github.com/cosmos/cosmos-sdk/pull/21822) This depinject module now only provides txconfig and tx config options. `x/validate` now handles providing ante/post handlers, alongside tx validators for v2. The corresponding app config options have been removed from the depinject module config.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
x/auth/proto/buf.lockis excluded by!**/*.lockx/auth/types/accounts.pb.gois excluded by!**/*.pb.go
Files selected for processing (40)
- .github/CODEOWNERS (2 hunks)
- .github/pr_labeler.yml (1 hunks)
- CHANGELOG.md (2 hunks)
- UPGRADING.md (4 hunks)
- api/cosmos/auth/v1beta1/accounts.pulsar.go (16 hunks)
- api/cosmos/tx/config/v1/config.pulsar.go (2 hunks)
- api/cosmos/validate/module/v1/module.pulsar.go (1 hunks)
- proto/cosmos/tx/config/v1/config.proto (0 hunks)
- proto/cosmos/validate/module/v1/module.proto (1 hunks)
- simapp/app_config.go (3 hunks)
- simapp/simd/cmd/testnet_test.go (1 hunks)
- simapp/v2/app_config.go (3 hunks)
- tests/e2e/auth/keeper/app_config.go (1 hunks)
- tests/e2e/baseapp/block_gas_test.go (1 hunks)
- tests/integration/bank/app_test.go (1 hunks)
- tests/integration/distribution/appconfig.go (1 hunks)
- tests/integration/evidence/app_config.go (1 hunks)
- tests/integration/mint/app_config.go (1 hunks)
- tests/integration/protocolpool/app_config.go (1 hunks)
- tests/integration/runtime/query_test.go (1 hunks)
- tests/integration/server/grpc/out_of_gas_test.go (1 hunks)
- tests/integration/server/grpc/server_test.go (1 hunks)
- tests/integration/slashing/app_config.go (1 hunks)
- tests/integration/slashing/slashing_test.go (1 hunks)
- tests/integration/staking/app_config.go (1 hunks)
- testutil/configurator/configurator.go (2 hunks)
- testutil/types.go (1 hunks)
- x/README.md (1 hunks)
- x/accounts/defaults/base/account.go (1 hunks)
- x/auth/keeper/grpc_query.go (1 hunks)
- x/auth/proto/buf.gen.gogo.yaml (0 hunks)
- x/auth/proto/buf.gen.pulsar.yaml (0 hunks)
- x/auth/proto/buf.yaml (0 hunks)
- x/auth/tx/README.md (2 hunks)
- x/auth/tx/config/depinject.go (3 hunks)
- x/tx/README.md (5 hunks)
- x/validate/README.md (1 hunks)
- x/validate/depinject.go (1 hunks)
- x/validate/keys.go (1 hunks)
- x/validate/module.go (1 hunks)
Files not reviewed due to no reviewable changes (4)
- proto/cosmos/tx/config/v1/config.proto
- x/auth/proto/buf.gen.gogo.yaml
- x/auth/proto/buf.gen.pulsar.yaml
- x/auth/proto/buf.yaml
Files skipped from review due to trivial changes (7)
- .github/CODEOWNERS
- api/cosmos/tx/config/v1/config.pulsar.go
- api/cosmos/validate/module/v1/module.pulsar.go
- x/README.md
- x/accounts/defaults/base/account.go
- x/validate/keys.go
- x/validate/module.go
Additional context used
Path-based instructions (27)
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"api/cosmos/auth/v1beta1/accounts.pulsar.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.simapp/simd/cmd/testnet_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"simapp/v2/app_config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/e2e/auth/keeper/app_config.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/e2e/baseapp/block_gas_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/bank/app_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/distribution/appconfig.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/evidence/app_config.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/mint/app_config.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/protocolpool/app_config.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/runtime/query_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/server/grpc/out_of_gas_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/server/grpc/server_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/slashing/app_config.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/slashing/slashing_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/staking/app_config.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"testutil/configurator/configurator.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testutil/types.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/keeper/grpc_query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/tx/README.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.x/tx/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/validate/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/validate/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
LanguageTool
UPGRADING.md
[grammar] ~325-~325: Comparison is written ‘the same … as’.
Context: ...ly the case). For the same instructions than for legacy app wiring to enable sign mo...(THE_SAME_AS)
[uncategorized] ~514-~514: Possible missing comma found.
Context: ...rs, no need to use this module. You can however always extend them by adding extra tx v...(AI_HYDRA_LEO_MISSING_COMMA)
x/auth/tx/README.md
[grammar] ~76-~76: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive.
Context: ...ject module. The depinject module is to outputs theTxConfigandTxConfigOptionsfo...(TO_NON_BASE)
x/validate/README.md
[uncategorized] ~4-~4: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...quired when using runtime and runtime v2 and you want to make use of the pre-defined...(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...dators. ::: Thex/validateis an app module solely there to setup ante/post handler...(AI_HYDRA_LEO_MISSING_COMMA)
buf
proto/cosmos/validate/module/v1/module.proto
5-5: import "cosmos/app/v1alpha1/module.proto": file does not exist
(COMPILE)
Markdownlint
x/auth/tx/README.md
23-23: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
24-24: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
25-25: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
26-26: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
27-27: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
28-28: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
29-29: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
30-30: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
31-31: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
32-32: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
33-33: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
34-34: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
35-35: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
36-36: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
37-37: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
38-38: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
39-39: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
40-40: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
41-41: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
x/tx/README.md
23-23: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
24-24: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
25-25: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
26-26: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
27-27: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
28-28: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
29-29: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
30-30: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
31-31: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
32-32: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
Additional comments not posted (42)
proto/cosmos/validate/module/v1/module.proto (2)
8-12: LGTM!The introduction of the
Modulemessage as a configuration object for thex/validatemodule is a great addition. It enhances the modularity and reusability of validation logic within the Cosmos SDK.
5-5: Verify the import statement.The static analysis tool has flagged the import statement
import "cosmos/app/v1alpha1/module.proto";indicating that the file does not exist.Please verify if the file exists at the specified path and make necessary corrections if required.
Verification successful
Import statement verified: File exists with a slight path difference
The imported file
cosmos/app/v1alpha1/module.protoexists in the repository, but it's located atproto/cosmos/app/v1alpha1/module.proto. The current import statement might work as-is, depending on how import paths are resolved in your project. However, you may want to consider updating the import statement to include theproto/prefix for consistency.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the imported file exists. # Test: Check if the file exists. Expect: File found. fd --no-ignore --type file --full-path "cosmos/app/v1alpha1/module.proto"Length of output: 112
Tools
buf
5-5: import "cosmos/app/v1alpha1/module.proto": file does not exist
(COMPILE)
tests/integration/mint/app_config.go (1)
22-22: Excellent addition of the validation module to the application configuration!The inclusion of
configurator.ValidateModule()in theAppConfiginitialization is a great step towards enhancing the robustness and integrity of the application. This module will likely be responsible for validating transactions and other operations, rejecting any invalid ones and maintaining data correctness.This change aligns with the best practice of validating inputs and ensuring data integrity in the application. Well done!
tests/e2e/auth/keeper/app_config.go (1)
23-23: LGTM! The addition of theValidateModule()enhances the application's configuration management.The inclusion of the
configurator.ValidateModule()in theAppConfigvariable initialization is a positive change that aligns with the PR objectives of introducing the newx/validatemodule. This addition incorporates validation capabilities into the application's configuration, ensuring that the settings adhere to expected formats and constraints. By validating the configuration, the application can potentially prevent runtime errors related to misconfigured settings, thereby improving its overall robustness and reliability.tests/integration/evidence/app_config.go (1)
24-24: LGTM!The addition of
configurator.ValidateModule()to theAppConfigvariable initialization is a positive change that enhances the application's configuration management process by integrating validation capabilities. This change is likely to improve the application's robustness by ensuring that the configuration adheres to certain rules or constraints, thereby maintaining data integrity and correctness.The change is well-integrated into the existing code and is unlikely to have any negative impact on the application's functionality.
testutil/types.go (1)
7-26: LGTM! The addition of the new module name constant and the formatting enhancements improve the codebase.The introduction of the
ValidateModuleNameconstant suggests an expansion of the SDK's capabilities, potentially adding new validation functionalities. This change aligns with the PR objectives mentioned in the summary.The reformatting of the module name constants enhances the readability and maintainability of the code, making it easier for developers to reference and utilize these constants across the SDK.
Great work on improving both the functionality and the code quality!
tests/integration/protocolpool/app_config.go (1)
24-24: LGTM!The addition of
configurator.ValidateModule()to theAppConfigvariable initialization is a positive change that aligns with the PR objective of introducing thex/validatemodule to improve the validation process within the SDK. This change enhances the application's robustness by incorporating validation logic into its configuration.tests/integration/distribution/appconfig.go (1)
24-24: Approve the addition of the validation module.The introduction of the
ValidateModule()in theAppConfiginitialization is a good practice, as it can help ensure that configurations adhere to specified rules or constraints, potentially improving robustness and error handling.Please verify if there are any dependencies or downstream impacts of adding this module. Consider running the following script to search for potential usage or references to the
ValidateModule()across the codebase:Verification successful
Confirm the addition of ValidateModule() as consistent and appropriate
The addition of
configurator.ValidateModule()in theAppConfiginitialization is consistent with the usage pattern observed across multiple test files and app configurations in the codebase. This module appears to be a standard component in the setup process for tests and possibly for the actual application.Given its widespread use and consistent integration alongside other modules, it's unlikely to introduce any unexpected behavior or have negative downstream impacts. The change aligns with the existing practices in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage or references to the `ValidateModule()` rg --type go -i -C 5 'ValidateModule\(\)'Length of output: 11315
tests/integration/staking/app_config.go (1)
25-25: LGTM! The addition of the validation module is a positive change.The introduction of the
configurator.ValidateModule()in theAppConfigis a straightforward and beneficial modification. This module is likely to improve the application's error handling and ensure data integrity by validating certain aspects of its operation.The change enhances the overall robustness of the application without introducing any apparent issues or complexities.
tests/integration/slashing/app_config.go (1)
26-26: LGTM! The addition of theValidateModuleenhances the application's robustness.The inclusion of the
configurator.ValidateModule()in theAppConfigis a positive change that strengthens the application's validation capabilities. This module will likely ensure that transactions and states within the application adhere to specified rules and constraints, thereby maintaining data integrity and correctness.By integrating this validation module into the application's configuration, error handling and validation processes during runtime should be improved. This proactive approach enhances the overall robustness of the application.
x/validate/README.md (4)
1-8: The module description is clear and informative.The overview of the
x/validatemodule effectively communicates its purpose and key features. It clearly explains how the module facilitates the setup of ante/post handlers and global transaction validators in runtime applications, and how it leverages dependency injection to automatically integrate these components.Tools
LanguageTool
[uncategorized] ~4-~4: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...quired when using runtime and runtime v2 and you want to make use of the pre-defined...(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...dators. ::: Thex/validateis an app module solely there to setup ante/post handler...(AI_HYDRA_LEO_MISSING_COMMA)
9-24: The "Extra TxValidators" section provides valuable guidance.The section offers a clear explanation of how developers can add extra transaction validators to their application, along with a relevant example use case. The included code snippet is a helpful reference for configuring custom validators using
depinjectin theapp.gofile.
26-35: The "Storage" section provides essential information.The section clearly communicates that the
x/validatemodule does not have a store key and reminds developers to include the module name in theSkipStoreKeysruntime config. The provided code snippet is a helpful reference for correctly adding the module name to theSkipStoreKeysarray.
7-7: Ignore the static analysis hint about a missing comma.The hint suggesting a possible missing comma in the sentence "The
x/validateis an app module solely there to setup ante/post handlers on a runtime app (via baseapp options) and the global tx validators on a runtime/v2 app (via app module)." is a false positive. The sentence is a single independent clause with multiple prepositional phrases, and there is no grammatical requirement for a comma in this structure. Adding a comma may actually disrupt the flow and readability of the sentence.Tools
LanguageTool
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...dators. ::: Thex/validateis an app module solely there to setup ante/post handler...(AI_HYDRA_LEO_MISSING_COMMA)
.github/pr_labeler.yml (1)
83-84: LGTM!The addition of the
C:x/validatelabel for thex/validate/**/*path aligns with the PR objective of introducing the newx/validatemodule. This change enhances the existing labeling structure by ensuring that contributions to the validation aspect of the project can be easily identified and managed.simapp/simd/cmd/testnet_test.go (1)
37-37: LGTM!The addition of
configurator.ValidateModule()to the list of modules being configured for the test is a positive change. It ensures that the newx/validatemodule is included in the test setup, which aligns with the PR objective and enhances the robustness of the tests.tests/integration/server/grpc/out_of_gas_test.go (1)
48-48: Enhancing test suite with validation capabilitiesThe addition of
configurator.ValidateModule()to the list of modules being initialized in theSetupSuitefunction is a positive change that improves the robustness of the integration tests. By incorporating validation capabilities, the test suite can now cover scenarios that involve validation checks on transactions or operations. This change aligns with best practices of comprehensive testing and enhances the overall quality of the test suite.tests/integration/runtime/query_test.go (1)
49-49: LGTM! The addition of the validation module enhances the integration tests.The inclusion of
configurator.ValidateModule()in the test fixture setup aligns with the PR objective of introducing the newx/validatemodule to improve the validation process within the SDK.This change could enable more comprehensive testing scenarios that verify certain conditions or constraints are met during test execution, thereby improving the robustness and reliability of the integration tests.
tests/integration/slashing/slashing_test.go (1)
70-70: LGTM! Adding validation to the test setup is a good enhancement.Including the
ValidateModulein the app configuration for the integration test is a positive change. It ensures that the slashing messages being tested adhere to the expected validation rules, thereby improving the robustness and reliability of the tests. Catching any validation issues early in the testing phase helps maintain a high quality codebase.The
TestSlashingMsgsfunction provides sufficient coverage for the core slashing messages (MsgCreateValidatorandMsgUnjail). It tests the success scenario of creating a validator and the failure scenario of unjailing an unknown validator, covering the key functionalities.tests/e2e/baseapp/block_gas_test.go (1)
91-91: LGTM!The addition of the
ValidateModuleto the app configuration is a good change. It ensures that the validation functionality is available for use in the tests, thereby improving the robustness and reliability of the test suite.x/auth/keeper/grpc_query.go (2)
234-234: Verify that theBasefield provides the necessary account information.The condition for checking account existence has been updated to use
xAccount.Baseinstead ofxAccount.Info. Ensure that theBasefield contains all the required account information for the desired functionality.
237-237: Ensure that theBasefield contains the expected account details.The response now returns
xAccount.Baseinstead ofxAccount.Info. Verify that theBasefield provides all the necessary account information expected by the calling code.x/auth/tx/README.md (3)
80-87: LGTM!The "Storage" section provides clear information about the module's lack of a store key and includes a helpful code snippet to illustrate the configuration of
SkipStoreKeys. The changes in this section are accurate and informative.
Line range hint
88-137: LGTM!The "CLI" section is well-documented and provides detailed information about the CLI commands for querying transactions and blocks. The examples are clear and cover various scenarios, making it easy for users to understand and use the CLI commands effectively. The changes in this section are comprehensive and informative.
Tools
LanguageTool
[grammar] ~76-~76: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive.
Context: ...ject module. The depinject module is to outputs theTxConfigandTxConfigOptionsfo...(TO_NON_BASE)
Line range hint
138-235: LGTM!The "gRPC" section provides detailed information about the gRPC endpoints for encoding and decoding transactions. The examples are well-documented and include sample requests and responses, making it easy for users to understand and interact with the gRPC endpoints. The changes in this section are comprehensive and informative.
Tools
Markdownlint
23-23: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
24-24: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
25-25: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
26-26: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
27-27: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
28-28: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
29-29: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
30-30: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
31-31: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
32-32: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
33-33: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
34-34: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
35-35: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
36-36: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
37-37: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
38-38: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
39-39: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
40-40: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
41-41: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
tests/integration/server/grpc/server_test.go (1)
59-59: LGTM! The addition of theValidateModulealigns with the PR objectives.The inclusion of the
configurator.ValidateModule()in the test suite setup is a positive change that incorporates validation functionality into the testing framework. This addition has the potential to enhance the robustness of the tests by ensuring data integrity and correctness.simapp/app_config.go (4)
31-31: LGTM!The import statement for the
validatemodulev1module is correctly added.
73-78: LGTM!The import statements for the
authtxconfigandvalidatemodules are correctly added.
186-187: LGTM!Adding the
authtxconfig.DepinjectModuleNameandvalidate.ModuleNameto theSkipStoreKeyslist is a valid configuration for modules that do not require a store key.
191-198: LGTM!The module configurations for the
authtxconfig.DepinjectModuleNameandvalidate.ModuleNamemodules are correctly added.
- The
authtxconfig.DepinjectModuleNamemodule is configured as a depinject module to provide transaction configuration.- The
validate.ModuleNamemodule is configured as an application module for validation functionality.These configurations align with the overall changes to integrate the
x/validatemodule and refactor the transaction configuration.simapp/v2/app_config.go (4)
31-31: LGTM!The import statement for the validation module is correctly added.
72-73: LGTM!The import statement for the
authtxconfigmodule is correctly added.
188-193: LGTM!The
SkipStoreKeysfield is correctly updated to include theauthtxconfig.DepinjectModuleNameandvalidate.ModuleName. This change aligns with the design decision to streamline the module's interaction with the state store.
196-203: LGTM!The new entries in the configuration list correctly integrate the
authtxconfig.DepinjectModuleNameandvalidate.ModuleNamemodules with their respective configurations. These changes enhance the application's modularity by incorporating validation capabilities and refining transaction handling.testutil/configurator/configurator.go (2)
198-205: LGTM!The
ValidateModulefunction correctly registers the validation module and wraps its configuration using theappconfig.WrapAnyfunction. The module name is defined in thetestutilpackage, which is a good practice for consistency. The function follows the same pattern as other module configuration functions in the file.
191-195: Verify the module name change in the codebase.The
TxModulefunction correctly registers the transaction module and wraps its configuration using theappconfig.WrapAnyfunction. The module name has been changed to indicate a shift in focus towards the authentication transaction configuration. The function follows the same pattern as other module configuration functions in the file.Run the following script to verify the module name usage:
Verification successful
Module name change verified and properly implemented.
The change from
testutil.TxModuleNametotestutil.AuthTxConfigDepinjectModuleNamehas been successfully implemented:
- The old module name has been completely removed from the codebase.
- The new module name is used consistently in the
testutil/configurator/configurator.gofile.- The constant
AuthTxConfigDepinjectModuleNameis properly defined intestutil/types.gowith the value "tx".The change appears to be isolated to the specific file under review, and there are no unexpected usages of the new module name throughout the codebase. The
TxModule()function, which is used in various test configurations, is not directly affected by this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the transaction module match the new name. # Test: Search for the old module name. Expect: No occurrences. rg --type go $'testutil\.TxModuleName' # Test: Search for the new module name. Expect: Only occurrences in the intended locations. rg --type go $'testutil\.AuthTxConfigDepinjectModuleName'Length of output: 321
Script:
#!/bin/bash # Description: Further verify the transaction module references and constant definition # Search for any references to 'tx' or 'transaction' module rg --type go -i '(tx|transaction).*module' # Look for the definition of the new module name constant rg --type go 'AuthTxConfigDepinjectModuleName\s*=' # Check for any remaining references to 'TxModule' function or variable rg --type go 'TxModule'Length of output: 85159
tests/integration/bank/app_test.go (1)
100-100: LGTM! The addition ofValidateModuleenhances the test suite.The inclusion of
configurator.ValidateModule()in the test suite configuration is a positive change that is likely to improve the robustness of the testing process by incorporating validation functionalities. This addition follows the existing pattern of module configuration and is placed appropriately in the list of configurations passed tosimtestutil.SetupWithConfiguration.api/cosmos/auth/v1beta1/accounts.pulsar.go (2)
375-375: LGTM!The renaming of the
Accountfield toBaseenhances clarity and aligns with theBaseAccounttype. The updated comments also provide better context for the field's purpose. The changes are consistent across the struct and its methods.Also applies to: 943-944, 996-999
973-977: LGTM!The
GetBasemethod has been appropriately updated to return theBasefield, aligning with the struct field renaming. The logic remains unchanged.x/auth/tx/config/depinject.go (3)
28-29: Module Name Definition is Clear and CorrectThe
DepinjectModuleNameconstant is appropriately defined and clearly indicates the module name used for dependency injection.
41-48: Struct Fields inModuleInputsAre Well-DefinedThe
ModuleInputsstruct fields are properly declared with clear types, and the use ofoptional:"true"tags forCustomSignModeHandlersandCustomGetSignersis correctly applied for optional dependencies.
90-93: Correctly Setting Transaction Encoder and DecoderThe
BaseAppOptionfunction correctly sets theTxDecoderandTxEncoder, ensuring that transactions are properly encoded and decoded within the application.
| * [`x/auth/tx`](#xauthtx) | ||
| * [Abstract](#abstract) | ||
| * [Contents](#contents) | ||
| * [Transactions](#transactions) | ||
| * [`TxConfig`](#txconfig) | ||
| * [`TxBuilder`](#txbuilder) | ||
| * [`TxEncoder`/ `TxDecoder`](#txencoder-txdecoder) | ||
| * [Depinject \& App Module](#depinject--app-module) | ||
| * [Client](#client) | ||
| * [`x/auth/tx/config`](#xauthtxconfig) | ||
| * [Storage](#storage) | ||
| * [Client](#client) | ||
| * [CLI](#cli) | ||
| * [Query](#query) | ||
| * [Transactions](#transactions-1) | ||
| * [`encode`](#encode) | ||
| * [`decode`](#decode) | ||
| * [gRPC](#grpc) | ||
| * [`TxDecode`](#txdecode) | ||
| * [`TxEncode`](#txencode) | ||
| * [`TxDecodeAmino`](#txdecodeamino) | ||
| * [`TxEncodeAmino`](#txencodeamino) |
There was a problem hiding this comment.
Fix the indentation in the "Contents" section.
The list items in the "Contents" section have inconsistent indentation. Please ensure that the indentation follows a consistent pattern to improve readability and maintain a clear visual hierarchy.
Tools
Markdownlint
23-23: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
24-24: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
25-25: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
26-26: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
27-27: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
28-28: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
29-29: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
30-30: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
31-31: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
32-32: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
33-33: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
34-34: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
35-35: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
36-36: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
37-37: Expected: 8; Actual: 4
Unordered list indentation(MD007, ul-indent)
38-38: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
39-39: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
40-40: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
41-41: Expected: 12; Actual: 6
Unordered list indentation(MD007, ul-indent)
(cherry picked from commit 763e4d6) # Conflicts: # .github/CODEOWNERS # CHANGELOG.md # UPGRADING.md # api/cosmos/auth/v1beta1/accounts.pulsar.go # api/cosmos/tx/config/v1/config.pulsar.go # simapp/app_config.go # x/tx/README.md
Description
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
x/validatemodule for transaction validation and handler setup.Bug Fixes
Documentation
x/validatemodule and its functionalities in the README.x/auth/txdocumentation for better navigation.Refactor
txtovalidateto better reflect its purpose.Chores