feat(x/auth/tx): extend app module + docs #21409
Conversation
WalkthroughWalkthroughThe changes involve updates to the Cosmos SDK's Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add Documentation and Community
|
julienrbrt
added
the
backport/v0.52.x
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- x/auth/module.go (1 hunks)
- x/auth/tx/README.md (2 hunks)
- x/auth/tx/config/depinject.go (4 hunks)
- x/auth/tx/config/module.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/auth/module.go
Additional context used
Path-based instructions (3)
x/auth/tx/config/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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/auth/tx/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
x/auth/tx/README.md
[misspelling] ~66-~66: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...is there to setup ante/post handlers on an runtime app (via baseapp options) and t...(EN_A_VS_AN)
Additional comments not posted (9)
x/auth/tx/config/module.go (4)
11-12: LGTM!The type alias
TxValidatoris correctly defined.The code changes are approved.
31-39: LGTM!The constructor function
NewAppModulecorrectly initializes theAppModulestruct with the provided parameters.The code changes are approved.
42-43: LGTM!The method
IsAppModuleis correctly implemented to satisfy theappmodule.AppModuleinterface requirements.The code changes are approved.
45-46: LGTM!The method
IsOnePerModuleTypeis correctly implemented to satisfy theappmodule.AppModuleinterface requirements.The code changes are approved.
x/auth/tx/config/depinject.go (4)
59-59: LGTM!The addition of
ExtraTxValidatorsenhances the flexibility of the module by allowing extra transaction validators.The code changes are approved.
65-65: LGTM!The addition of
Moduleimproves the organization of the output structure.The code changes are approved.
153-158: LGTM!The function
ProvideModulecorrectly initializes theModuleOutputsstruct with the provided parameters.The code changes are approved.
Line range hint
161-178: LGTM!The function
newAnteHandlercorrectly initializes theanteHandlerwith the provided parameters.The code changes are approved.
x/auth/tx/README.md (1)
Line range hint
71-278: LGTM!The documentation is clear and comprehensive.
The code changes are approved.
Tools
LanguageTool
[misspelling] ~66-~66: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...is there to setup ante/post handlers on an runtime app (via baseapp options) and t...(EN_A_VS_AN)
x/auth/tx/config/module.go
Outdated
| // TxValidator implements appmodule.HasTxValidator. | ||
| func (a AppModule) TxValidator(ctx context.Context, tx transaction.Tx) error { | ||
| for _, validator := range a.txValidators { | ||
| if err := validator(ctx, tx); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return a.sigVerification.ValidateTx(ctx, tx) |
There was a problem hiding this comment.
Add a check for the presence of txValidators.
The method correctly iterates over the txValidators slice and validates the transaction. However, adding a check for the presence of txValidators before iterating can improve robustness.
Apply this diff to add the check:
func (a AppModule) TxValidator(ctx context.Context, tx transaction.Tx) error {
+ if len(a.txValidators) == 0 {
+ return nil
+ }
for _, validator := range a.txValidators {
if err := validator(ctx, tx); err != nil {
return err
}
}
return a.sigVerification.ValidateTx(ctx, tx)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TxValidator implements appmodule.HasTxValidator. | |
| func (a AppModule) TxValidator(ctx context.Context, tx transaction.Tx) error { | |
| for _, validator := range a.txValidators { | |
| if err := validator(ctx, tx); err != nil { | |
| return err | |
| } | |
| } | |
| return a.sigVerification.ValidateTx(ctx, tx) | |
| func (a AppModule) TxValidator(ctx context.Context, tx transaction.Tx) error { | |
| if len(a.txValidators) == 0 { | |
| return nil | |
| } | |
| for _, validator := range a.txValidators { | |
| if err := validator(ctx, tx); err != nil { | |
| return err | |
| } | |
| } | |
| return a.sigVerification.ValidateTx(ctx, tx) | |
| } |
| The `x/auth/tx/config` contains a depinject module and app module. | ||
| The depinject module is there to setup ante/post handlers on an runtime app (via baseapp options) and the tx validator on the runtime/v2 app (via app module). It as well outputs the `TxConfig` and `TxConfigOptions` for the app. |
There was a problem hiding this comment.
Correct grammatical error.
The sentence "is there to setup ante/post handlers on an runtime app" contains a grammatical error. Use "a" instead of "an".
Apply this diff to fix the grammatical error:
The depinject module is there to setup ante/post handlers on an runtime app (via baseapp options) and the tx validator on the runtime/v2 app (via app module). It as well outputs the `TxConfig` and `TxConfigOptions` for the app.
+The depinject module is there to setup ante/post handlers on a runtime app (via baseapp options) and the tx validator on the runtime/v2 app (via app module). It as well outputs the `TxConfig` and `TxConfigOptions` for the app.Committable suggestion was skipped due to low confidence.
Tools
LanguageTool
[misspelling] ~66-~66: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...is there to setup ante/post handlers on an runtime app (via baseapp options) and t...(EN_A_VS_AN)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/auth/tx/config/depinject.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/auth/tx/config/depinject.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- x/auth/tx/config/depinject.go (4 hunks)
- x/auth/tx/config/module.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/auth/tx/config/module.go
Additional context used
Path-based instructions (1)
x/auth/tx/config/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (3)
x/auth/tx/config/depinject.go (3)
17-17: LGTM!The import of
appmodule/v2is necessary for the new modular approach in transaction handling.The code changes are approved.
54-62: LGTM!The addition of the
ExtraTxValidatorsfield enhances the flexibility of the module by allowing extra transaction validators.The code changes are approved.
156-161: LGTM!The change to
ModuleOutputsreflects a shift towards a more modular approach in handling transaction validation.The code changes are approved.
(cherry picked from commit 355f4d7)
Description
Allow users to define tx validator in their app.go without creating a module.
This can be done by supplying them to depinject as
[]TxValidator{}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/auth/txpackage, clarifying dependency injection and application module functionalities.Bug Fixes