docs: demonstrate how to define hooks using depinject #21892
Conversation
WalkthroughWalkthroughThe changes introduce a new hooks injection mechanism using dependency injection in the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Keeper
participant Hooks
App->>Keeper: SetHooks(hooks)
Keeper->>Hooks: AfterIncreaseCount(ctx, n)
Hooks-->>Keeper: return nil
Keeper->>App: return success
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (12)
testutil/x/counter/keeper/hooks.go (2)
7-9: Consider adding documentation for the Hooks struct and its field.While the struct and field names are clear and follow Go naming conventions, adding documentation comments would improve code readability and maintainability. Consider adding a comment explaining the purpose of the
Hooksstruct and theAfterCounterIncreasedfield.Example:
// Hooks defines the hook functionality for the counter keeper. type Hooks struct { // AfterCounterIncreased indicates whether the counter has been increased. AfterCounterIncreased bool }
11-14: Improve method documentation and consider unused parameter.
- Add documentation for the
AfterIncreaseCountmethod to explain its purpose and parameters.- The
nparameter is currently unused. If it's intentional for this test utility, consider adding a comment explaining why, or remove it if it's not needed.Example improvement:
// AfterIncreaseCount is a hook that's called after the counter is increased. // It sets the AfterCounterIncreased flag to true. // // ctx is the context of the operation (unused in this implementation). // n is the amount by which the counter was increased (unused in this implementation). func (h *Hooks) AfterIncreaseCount(ctx context.Context, n int64) error { h.AfterCounterIncreased = true return nil }If
nis truly not needed, you could simplify the method signature:func (h *Hooks) AfterIncreaseCount(ctx context.Context) error {testutil/x/counter/types/expected_keepers.go (2)
5-7: LGTM: CounterHooks interface is well-defined.The
CounterHooksinterface is clear and aligns with the PR objective of demonstrating hook usage with depinject. The method signature is appropriate for its purpose.Consider adding a brief comment to describe the purpose of the
CounterHooksinterface and its method. For example:// CounterHooks defines the hook interface for the counter module. type CounterHooks interface { // AfterIncreaseCount is called after the count is increased. AfterIncreaseCount(ctx context.Context, newCount int64) error }
9-12: LGTM: CounterHooksWrapper and IsOnePerModuleType method are correctly implemented.The
CounterHooksWrapperstruct and itsIsOnePerModuleTypemethod are well-implemented to work with the depinject framework, aligning with the PR objectives.Consider adding a brief comment to describe the purpose of the
CounterHooksWrapperstruct. For example:// CounterHooksWrapper wraps CounterHooks and implements depinject.OnePerModuleType. type CounterHooksWrapper struct{ CounterHooks }testutil/x/counter/types/hooks.go (2)
8-11: LGTM: Type assertion and definition are well-structured.The type assertion ensures that
MultiCounterHooksimplements theCounterHooksinterface, which is a good practice. TheMultiCounterHookstype is correctly defined as a slice ofCounterHooks.Consider expanding the comment to provide more context:
- // MultiCounterHooks is a slice of hooks to be called in sequence. + // MultiCounterHooks is a slice of CounterHooks to be called in sequence. + // It implements the CounterHooks interface.
18-26: LGTM: AfterIncreaseCount method is well-implemented with robust error handling.The method correctly iterates over all hooks and aggregates errors using
errors.Join. This approach ensures all hooks are executed and all errors are reported.Consider a small optimization to avoid unnecessary allocations:
func (ch MultiCounterHooks) AfterIncreaseCount(ctx context.Context, newCount int64) error { - var errs error + var errs []error for i := range ch { - errs = errors.Join(errs, ch[i].AfterIncreaseCount(ctx, newCount)) + if err := ch[i].AfterIncreaseCount(ctx, newCount); err != nil { + errs = append(errs, err) + } } - - return errs + return errors.Join(errs...) }This change avoids calling
errors.Joinfor every iteration and only allocates when errors occur.testutil/x/counter/README.md (4)
9-12: Enhance the hooks introduction and fix list formattingThe introduction to hooks could be more informative. Consider adding a brief explanation of what hooks are and their benefits in the context of the
x/countermodule.Also, there's a minor formatting issue with the ordered list. To ensure proper rendering, add a blank line before the list starts.
Consider updating the hooks section as follows:
## Hooks Hooks allow you to extend the functionality of the `x/counter` module by injecting custom logic at specific points in its execution. This can be useful for implementing cross-module communication or adding custom behavior. The module supports hook injection using `depinject`. Follow these steps to implement hooks: 1. Define the hook interface and a wrapper implementing `depinject.OnePerModuleType`:
13-24: LGTM! Minor formatting suggestionThe code for defining the
CounterHooksinterface andCounterHooksWrapperstruct looks good. It's clear and follows Go conventions.For better readability in the Markdown file, consider adding a blank line before and after the code block:
```go type CounterHooks interface { AfterIncreaseCount(ctx context.Context, newCount int64) error } type CounterHooksWrapper struct{ CounterHooks } // IsOnePerModuleType implements the depinject.OnePerModuleType interface. func (CounterHooksWrapper) IsOnePerModuleType() {}<details> <summary>Tools</summary> <details> <summary>Markdownlint</summary><blockquote> 14-14: null Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) --- 24-24: null Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) --- 13-13: null Lists should be surrounded by blank lines (MD032, blanks-around-lists) </blockquote></details> </details> --- `25-35`: **LGTM! Suggestions for improved formatting** The code for modifying the `Keeper` struct and adding the `SetHooks` method looks good. It's clear and follows Go conventions. Consider the following formatting improvements: 1. Add a blank line before and after the code block for better readability in Markdown. 2. Fix the list numbering to ensure proper rendering: ```markdown 2. Add a `CounterHooks` field to the keeper: ```go type Keeper struct { // ... hooks CounterHooks } func (k *Keeper) SetHooks(hooks ...CounterHooks) { k.hooks = hooks }
- Create a
depinjectinvoker function<details> <summary>Tools</summary> <details> <summary>Markdownlint</summary><blockquote> 25-25: Expected: 1; Actual: 2; Style: 1/1/1 Ordered list item prefix (MD029, ol-prefix) --- 26-26: null Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) --- 35-35: null Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) --- 25-25: null Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 25-25: null Lists should be surrounded by blank lines (MD032, blanks-around-lists) </blockquote></details> </details> --- `45-68`: **LGTM! Suggestions for improved formatting and clarity** The code snippet for injecting hooks during app initialization is helpful and provides a good example of how to use `depinject` with the `x/counter` module. Consider the following improvements: 1. Add a blank line before and after the code block for better readability in Markdown. 2. Fix the list numbering: ```markdown 4. Inject the hooks during app initialization: ```go appConfig = appconfig.Compose(&appv1alpha1.Config{ Modules: []*appv1alpha1.ModuleConfig{ // .... { Name: types.ModuleName, Config: appconfig.WrapAny(&types.Module{}), }, } }) appConfig = depinject.Configs( AppConfig(), runtime.DefaultServiceBindings(), depinject.Supply( logger, viper, map[string]types.CounterHooksWrapper{ "counter": types.CounterHooksWrapper{&types.Hooks{}}, }, ), )By following these steps, you can successfully integrate hooks into the
x/countermodule usingdepinject.3. Consider adding a brief explanation of what each part of the configuration does, especially the `depinject.Supply` section, to help users understand the purpose of each component. <details> <summary>Tools</summary> <details> <summary>Markdownlint</summary><blockquote> 45-45: Expected: 1; Actual: 4; Style: 1/1/1 Ordered list item prefix (MD029, ol-prefix) --- 46-46: null Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) --- 67-67: null Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) --- 45-45: null Lists should be surrounded by blank lines (MD032, blanks-around-lists) --- 45-45: null Lists should be surrounded by blank lines (MD032, blanks-around-lists) </blockquote></details> </details> </blockquote></details> <details> <summary>testutil/x/counter/depinject.go (1)</summary><blockquote> `65-65`: **Improve error message clarity** If retaining the existence check, consider enhancing the error message for clarity. Apply this diff to improve the error message: ```diff return fmt.Errorf("hook not found for module: %s", modName)testutil/x/counter/keeper/keeper.go (1)
99-106: SimplifySetHooksMethod SignatureReturning
*Keeperfrom theSetHooksmethod may not be necessary if method chaining is not being utilized. Simplifying the method signature enhances readability.Suggested change:
-func (k *Keeper) SetHooks(gh types.CounterHooks) *Keeper { +func (k *Keeper) SetHooks(gh types.CounterHooks) error { if k.hooks != nil { // existing check } k.hooks = gh - return k + return nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (8)
- CHANGELOG.md (1 hunks)
- testutil/x/counter/README.md (1 hunks)
- testutil/x/counter/depinject.go (4 hunks)
- testutil/x/counter/keeper/hooks.go (1 hunks)
- testutil/x/counter/keeper/keeper.go (3 hunks)
- testutil/x/counter/module.go (2 hunks)
- testutil/x/counter/types/expected_keepers.go (1 hunks)
- testutil/x/counter/types/hooks.go (1 hunks)
🧰 Additional context used
Path-based instructions (8)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"testutil/x/counter/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"testutil/x/counter/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testutil/x/counter/keeper/hooks.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testutil/x/counter/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testutil/x/counter/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testutil/x/counter/types/expected_keepers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testutil/x/counter/types/hooks.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Markdownlint
testutil/x/counter/README.md
25-25: Expected: 1; Actual: 2; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
36-36: Expected: 1; Actual: 3; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
45-45: Expected: 1; Actual: 4; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
14-14: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
24-24: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
26-26: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
35-35: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
37-37: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
44-44: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
46-46: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
67-67: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
13-13: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
25-25: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
25-25: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
36-36: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
36-36: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
45-45: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
45-45: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
🔇 Additional comments not posted (18)
testutil/x/counter/keeper/hooks.go (2)
1-5: LGTM: Package declaration and imports are correct.The package name
keeperis appropriate for the file's location and purpose. Thecontextimport is correctly formatted and necessary for thecontext.Contexttype used in the method signature.
1-14: LGTM: Implementation aligns well with PR objectives.The
hooks.gofile successfully demonstrates a simple hook implementation for the counter keeper, which aligns perfectly with the PR objectives and the linked issue #19897. This concise example effectively shows how to define hooks in a manner compatible with depinject, providing a clear template for other modules to follow.Key points:
- The
Hooksstruct provides a simple flag to track hook execution.- The
AfterIncreaseCountmethod demonstrates how to implement a hook function.- The overall structure is clear and easy to understand, making it an excellent example for other developers.
This implementation serves as a good starting point for more complex hook implementations in other modules.
testutil/x/counter/types/expected_keepers.go (2)
1-3: LGTM: Package declaration and imports are correct.The package name is appropriate, and the import of "context" is necessary for the
CounterHooksinterface.
1-12: Overall assessment: Excellent implementation of hooks with depinject.This file successfully demonstrates the usage of hooks with the depinject framework, addressing the objectives outlined in issue #19897. The code is well-structured, follows Go conventions, and provides a clear example for other modules to follow when implementing hooks with depinject.
Great job on this implementation!
testutil/x/counter/types/hooks.go (3)
1-7: LGTM: Package declaration and imports are appropriate.The package name 'types' is suitable for defining types and interfaces. The imports are correctly limited to the necessary standard library packages.
13-16: LGTM: Constructor function is well-implemented.The
NewMultiCounterHooksfunction is a clean and efficient constructor forMultiCounterHooks. It correctly uses variadic parameters to allow for flexible initialization.
1-26: Great implementation of multi-hook system for the counter module.This file successfully demonstrates how to define hooks in a manner compatible with depinject, addressing the objectives outlined in issue #19897. The implementation of
MultiCounterHooksprovides a flexible and extensible way to manage multiple hooks, which can be easily integrated with the depinject framework.Key points:
- The
MultiCounterHookstype implements theCounterHooksinterface, ensuring compatibility.- The constructor function
NewMultiCounterHooksallows for easy creation and injection of hooks.- The
AfterIncreaseCountmethod demonstrates how to execute multiple hooks sequentially while handling errors comprehensively.This implementation serves as a good example for other modules that need to implement hooks using depinject.
testutil/x/counter/module.go (3)
34-37: Approved: NewAppModule function updated to use pointer keeperThe change in the
NewAppModulefunction signature fromfunc NewAppModule(keeper keeper.Keeper)tofunc NewAppModule(keeper *keeper.Keeper)is correct and consistent with the earlier modification in theAppModulestruct. This ensures that theAppModuleis created with the proper keeper type.
Line range hint
1-52: Approved: Overall module structure and implementationThe module implementation follows good practices for Cosmos SDK modules:
- Proper interface implementations (
appmodule.AppModule,appmodule.HasRegisterInterfaces)- Correct registration of services in
RegisterServices- Implementation of
ConsensusVersionfor module versioning- Clear and concise implementation of required functions
The changes made to the keeper field and
NewAppModulefunction are consistent with the rest of the module's structure.
20-20: Approved: Keeper field changed to pointer typeThe change from
keeper keeper.Keepertokeeper *keeper.Keeperis appropriate and aligns with the PR objectives. Using a pointer can be more efficient and flexible, especially when working with dependency injection frameworks like depinject.To ensure this change doesn't introduce any issues, please verify the usage of the keeper throughout the module:
Verification successful
Verified: Keeper usage updated to pointer type
The
keeperis consistently used as a pointer throughout the module, aligning with the recent changes. No issues detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of keeper in the counter module # Search for keeper usage in the counter module rg --type go 'keeper\.' testutil/x/counterLength of output: 499
CHANGELOG.md (3)
Line range hint
1-55: Important updates in unreleased changesThe unreleased changes section contains several significant updates that developers should be aware of:
- A new
MsgCreatePermanentLockedAccounthas been added to the vesting module, along with a CLI method for creating permanent locked accounts.- The
x/govmodule now includes expedited proposals, which have been upstreamed from Osmosis.- There are improvements to the
keyringpackage, including support for importing hex keys and auto CLI for the node service.- Several modules have been moved to their own go.mod files, including x/nft, x/group, x/gov, x/distribution, x/slashing, x/authz, x/mint, and x/consensus.
These changes indicate ongoing modularization efforts and feature additions to the Cosmos SDK. Developers should review these changes carefully, especially if they are working with the affected modules.
Line range hint
57-401: Significant changes in v0.47.0The v0.47.0 release includes numerous breaking changes and improvements:
State Machine Breaking Changes:
- Several modules have undergone significant changes, including x/gov, x/group, x/staking, and x/auth/vesting.
- The
PreBlockfunction has been introduced in baseapp, which runs before the begin blocker of other modules.API Breaking Changes:
- Many modules have had their APIs updated, including changes to function signatures, removal of deprecated functions, and updates to protobuf definitions.
- The
sdk.Msginterface now inheritsproto.Message, which affects allsdk.Msgtypes.Client Breaking Changes:
- Several CLI commands have been renamed or had their behavior modified.
- The REST API has been largely removed in favor of gRPC endpoints.
Features and Improvements:
- New features include support for vote extensions, improvements to the mempool, and updates to various modules like x/gov and x/staking.
- There are significant performance improvements in areas such as coin operations and iterator performance.
Developers should carefully review these changes as they may require significant updates to existing applications built on the Cosmos SDK. Special attention should be paid to state machine breaking changes and API updates.
Line range hint
1-8183: Comprehensive and well-structured CHANGELOGThis CHANGELOG provides a detailed and well-organized history of changes for the Cosmos SDK. It covers multiple versions, with each version clearly delineating breaking changes, new features, improvements, and bug fixes. The level of detail provided is commendable, making it easier for developers to understand the impact of updates and plan their migration strategies.
For anyone working with the Cosmos SDK, regularly reviewing this CHANGELOG is crucial to stay informed about new features, performance improvements, and potential breaking changes. The clear categorization of changes (e.g., State Machine Breaking, API Breaking, Client Breaking) is particularly helpful for assessing the impact of updates on existing applications.
testutil/x/counter/depinject.go (3)
4-6: Appropriate imports addedThe imports of
"fmt","maps", and"slices"are necessary for the new functionality inInvokeSetHooks.
39-39: Verify the change ofKeeperto a pointer typeChanging
Keeperfrom a value to a pointer may have broader implications. Ensure thatkeeper.NewKeeperreturns a*keeper.Keeperand that all usages ofModuleOutputs.Keeperare updated to handle the pointer type appropriately.
62-68: 🛠️ Refactor suggestionSimplify loop by removing redundant map check
Since
modNameis derived fromcounterHookskeys, the check for existence is unnecessary. Removing the redundantokcheck simplifies the code.Apply this diff to simplify the loop:
for _, modName := range modNames { - hook, ok := counterHooks[modName] - if !ok { - return fmt.Errorf("can't find hooks for module %s", modName) - } + hook := counterHooks[modName] multiHooks = append(multiHooks, hook) }Likely invalid or redundant comment.
testutil/x/counter/keeper/keeper.go (2)
28-30: Appropriate Change:NewKeeperReturns a PointerChanging the
NewKeeperfunction to return a pointer toKeeperis appropriate given that the struct now contains mutable fields. This aligns with best practices in Go for managing mutable types.
72-75: Invocation of Hooks After Increasing CountThe addition of invoking
k.Hooks().AfterIncreaseCountafter updating the count is correctly implemented. This ensures that any registered hooks are executed, allowing for additional processing.
| 3. Create a `depinject` invoker function | ||
| ```go | ||
| func InvokeSetHooks(keeper *keeper.Keeper, counterHooks map[string]types.CounterHooksWrapper) error { | ||
| //... | ||
| keeper.SetHooks(multiHooks) | ||
| return nil | ||
| } | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Improve the depinject invoker function example
The InvokeSetHooks function signature looks correct, but the implementation details are missing. This might be confusing for users trying to implement the function.
Consider providing a more complete example of the InvokeSetHooks function:
func InvokeSetHooks(keeper *keeper.Keeper, counterHooks map[string]types.CounterHooksWrapper) error {
for _, hook := range counterHooks {
keeper.SetHooks(hook)
}
return nil
}Also, improve the formatting:
- Add a blank line before and after the code block.
- Fix the list numbering:
3. Create a `depinject` invoker function:
```go
func InvokeSetHooks(keeper *keeper.Keeper, counterHooks map[string]types.CounterHooksWrapper) error {
for _, hook := range counterHooks {
keeper.SetHooks(hook)
}
return nil
}- Inject the hooks during app initialization:
<details>
<summary>Tools</summary>
<details>
<summary>Markdownlint</summary><blockquote>
36-36: Expected: 1; Actual: 3; Style: 1/1/1
Ordered list item prefix
(MD029, ol-prefix)
---
37-37: null
Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
---
44-44: null
Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
---
36-36: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
---
36-36: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
</blockquote></details>
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| @@ -17,6 +21,7 @@ func (am AppModule) IsOnePerModuleType() {} | |||
| func init() { | |||
| appconfig.RegisterModule( | |||
| &types.Module{}, | |||
| appconfig.Invoke(InvokeSetHooks), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reordering appconfig.Provide before appconfig.Invoke
To ensure that all dependencies are available when invoked, it's customary to call appconfig.Provide before appconfig.Invoke in appconfig.RegisterModule.
Apply this diff to reorder the calls:
appconfig.RegisterModule(
&types.Module{},
+ appconfig.Provide(ProvideModule),
appconfig.Invoke(InvokeSetHooks),
- appconfig.Provide(ProvideModule),
)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.
| appconfig.Invoke(InvokeSetHooks), | |
| appconfig.RegisterModule( | |
| &types.Module{}, | |
| appconfig.Provide(ProvideModule), | |
| appconfig.Invoke(InvokeSetHooks), | |
| ) |
| if keeper == nil || counterHooks == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Return an error when dependencies are missing
Returning nil when keeper is nil or counterHooks is nil may mask underlying issues. Consider returning an error to indicate missing dependencies.
Apply this diff to return an error when dependencies are missing:
func InvokeSetHooks(keeper *keeper.Keeper, counterHooks map[string]types.CounterHooksWrapper) error {
- if keeper == nil || counterHooks == nil {
- return nil
- }
+ if keeper == nil {
+ return fmt.Errorf("keeper is nil")
+ }
+ if counterHooks == nil {
+ return fmt.Errorf("counterHooks is nil")
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if keeper == nil || counterHooks == nil { | |
| return nil | |
| } | |
| func InvokeSetHooks(keeper *keeper.Keeper, counterHooks map[string]types.CounterHooksWrapper) error { | |
| if keeper == nil { | |
| return fmt.Errorf("keeper is nil") | |
| } | |
| if counterHooks == nil { | |
| return fmt.Errorf("counterHooks is nil") | |
| } |
|
|
||
| hooks types.CounterHooks |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure Consistent Use of Pointer Receivers for Keeper Methods
The Keeper struct now contains a pointer field hooks, and methods have a mix of value and pointer receivers. To maintain consistency and prevent potential issues, it's recommended to use pointer receivers for all methods associated with the Keeper struct.
Suggested changes:
Update the method receivers to use pointers:
-var _ types.QueryServer = Keeper{}
+var _ types.QueryServer = &Keeper{}
-func (k Keeper) GetCount(ctx context.Context, _ *types.QueryGetCountRequest) (*types.QueryGetCountResponse, error) {
+func (k *Keeper) GetCount(ctx context.Context, _ *types.QueryGetCountRequest) (*types.QueryGetCountResponse, error) {
-var _ types.MsgServer = Keeper{}
+var _ types.MsgServer = &Keeper{}
-func (k Keeper) IncreaseCount(ctx context.Context, msg *types.MsgIncreaseCounter) (*types.MsgIncreaseCountResponse, error) {
+func (k *Keeper) IncreaseCount(ctx context.Context, msg *types.MsgIncreaseCounter) (*types.MsgIncreaseCountResponse, error) {Committable suggestion was skipped due to low confidence.
| if k.hooks != nil { | ||
| panic("cannot set governance hooks twice") | ||
| } |
There was a problem hiding this comment.
Avoid Using panic; Return an Error Instead in SetHooks
Using panic in the SetHooks method is discouraged as it can cause the entire program to exit unexpectedly. Instead, return an error to allow the caller to handle the situation gracefully.
Suggested change:
func (k *Keeper) SetHooks(gh types.CounterHooks) *Keeper {
if k.hooks != nil {
- panic("cannot set governance hooks twice")
+ return errors.New("cannot set hooks twice")
}
k.hooks = gh
- return k
+ return nil
}Committable suggestion was skipped due to low confidence.
| @@ -52,6 +52,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i | |||
| ### Improvements | |||
|
|
|||
| * (sims) [#21613](https://github.com/cosmos/cosmos-sdk/pull/21613) Add sims2 framework and factory methods for simpler message factories in modules | |||
| * (testutil/counter)[#21892](https://github.com/cosmos/cosmos-sdk/pull/21892) Add support for hooks injection with depinject. | |||
| @@ -5,3 +5,64 @@ sidebar_position: 1 | |||
| # `x/counter` | |||
|
|
|||
| Counter is a module used for testing purposes within the Cosmos SDK. | |||
|
|
|||
| ## Hooks | |||
There was a problem hiding this comment.
Nice, but I meant creating a page at build-modules/hooks on the website instead. No one will ever find this otherwise :D
Basically, if you could move that section under docs/build-modules/define-hooks that would be great.
We should have as well an intro about what is a hook, how to wire it (this) and link those pages: https://docs.cosmos.network/main/build/modules/epochs#hooks, https://docs.cosmos.network/main/build/modules/distribution#hooks
We can use code snippet, as go reference to link the counter module changes you've done here and example what everything does.
julienrbrt
changed the title
Description
Closes: #19897
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/countermodule.Improvements
Keeperstruct to support hooks for additional processing during count increases.Documentation