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

docs: demonstrate how to define hooks using depinject #21892

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be deleted


### Bug Fixes

Expand Down
61 changes: 61 additions & 0 deletions testutil/x/counter/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,64 @@ sidebar_position: 1
# `x/counter`

Counter is a module used for testing purposes within the Cosmos SDK.

## Hooks
Copy link
Member

@julienrbrt julienrbrt Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


The module supports hook injection using `depinject`. Follow these steps to implement hooks:

1. Define the hook interface and a wrapper implementing `depinject.OnePerModuleType`:
```go
type CounterHooks interface {
AfterIncreaseCount(ctx context.Context, newCount int64) error
}

type CounterHooksWrapper struct{ CounterHooks }

// IsOnePerModuleType implements the depinject.OnePerModuleType interface.
func (CounterHooksWrapper) IsOnePerModuleType() {}

```
2. Add a `CounterHooks` field to the keeper:
```go
type Keeper struct{
//...
hooks CounterHooks
}

func (k *Keeper) SetHooks(hooks ...CounterHooks){
k.hooks = hooks
}
```
3. Create a `depinject` invoker function
```go
func InvokeSetHooks(keeper *keeper.Keeper, counterHooks map[string]types.CounterHooksWrapper) error {
//...
keeper.SetHooks(multiHooks)
return nil
}

```
Comment on lines +36 to +44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

  1. Add a blank line before and after the code block.
  2. 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
}
  1. 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 -->

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/counter` module using `depinject`.
28 changes: 27 additions & 1 deletion testutil/x/counter/depinject.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package counter

import (
"fmt"
"maps"
"slices"

"cosmossdk.io/core/appmodule"
"cosmossdk.io/depinject"
"cosmossdk.io/depinject/appconfig"
Expand All @@ -17,6 +21,7 @@ func (am AppModule) IsOnePerModuleType() {}
func init() {
appconfig.RegisterModule(
&types.Module{},
appconfig.Invoke(InvokeSetHooks),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
appconfig.Invoke(InvokeSetHooks),
appconfig.RegisterModule(
&types.Module{},
appconfig.Provide(ProvideModule),
appconfig.Invoke(InvokeSetHooks),
)

appconfig.Provide(ProvideModule),
)
}
Expand All @@ -31,7 +36,7 @@ type ModuleInputs struct {
type ModuleOutputs struct {
depinject.Out

Keeper keeper.Keeper
Keeper *keeper.Keeper
Module appmodule.AppModule
}

Expand All @@ -44,3 +49,24 @@ func ProvideModule(in ModuleInputs) ModuleOutputs {
Module: m,
}
}

func InvokeSetHooks(keeper *keeper.Keeper, counterHooks map[string]types.CounterHooksWrapper) error {
if keeper == nil || counterHooks == nil {
return nil
}
Comment on lines +54 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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")
}


// Default ordering is lexical by module name.
// Explicit ordering can be added to the module config if required.
modNames := slices.Sorted(maps.Keys(counterHooks))
var multiHooks types.MultiCounterHooks
for _, modName := range modNames {
hook, ok := counterHooks[modName]
if !ok {
return fmt.Errorf("can't find hooks for module %s", modName)
}
multiHooks = append(multiHooks, hook)
}

keeper.SetHooks(multiHooks)
return nil
}
14 changes: 14 additions & 0 deletions testutil/x/counter/keeper/hooks.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package keeper

import (
"context"
)

type Hooks struct {
AfterCounterIncreased bool
}

func (h *Hooks) AfterIncreaseCount(ctx context.Context, n int64) error {
h.AfterCounterIncreased = true
return nil
}
30 changes: 28 additions & 2 deletions testutil/x/counter/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@ type Keeper struct {
appmodule.Environment

CountStore collections.Item[int64]

hooks types.CounterHooks
Comment on lines +24 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

func NewKeeper(env appmodule.Environment) Keeper {
func NewKeeper(env appmodule.Environment) *Keeper {
sb := collections.NewSchemaBuilder(env.KVStoreService)
return Keeper{
return &Keeper{
Environment: env,
CountStore: collections.NewItem(sb, collections.NewPrefix(0), "count", collections.Int64Value),
}
Expand Down Expand Up @@ -67,6 +69,10 @@ func (k Keeper) IncreaseCount(ctx context.Context, msg *types.MsgIncreaseCounter
return nil, err
}

if err := k.Hooks().AfterIncreaseCount(ctx, num+msg.Count); err != nil {
return nil, err
}

if err := k.EventService.EventManager(ctx).EmitKV(
"increase_counter",
event.NewAttribute("signer", msg.Signer),
Expand All @@ -78,3 +84,23 @@ func (k Keeper) IncreaseCount(ctx context.Context, msg *types.MsgIncreaseCounter
NewCount: num + msg.Count,
}, nil
}

// Hooks gets the hooks for counter Keeper
func (k *Keeper) Hooks() types.CounterHooks {
if k.hooks == nil {
// return a no-op implementation if no hooks are set
return types.MultiCounterHooks{}
}

return k.hooks
}

// SetHooks sets the hooks for counter
func (k *Keeper) SetHooks(gh types.CounterHooks) *Keeper {
if k.hooks != nil {
panic("cannot set governance hooks twice")
}
Comment on lines +100 to +102
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


k.hooks = gh
return k
}
4 changes: 2 additions & 2 deletions testutil/x/counter/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ var (

// AppModule implements an application module
type AppModule struct {
keeper keeper.Keeper
keeper *keeper.Keeper
}

// IsAppModule implements the appmodule.AppModule interface.
Expand All @@ -31,7 +31,7 @@ func (am AppModule) RegisterServices(registrar grpc.ServiceRegistrar) error {
}

// NewAppModule creates a new AppModule object
func NewAppModule(keeper keeper.Keeper) AppModule {
func NewAppModule(keeper *keeper.Keeper) AppModule {
return AppModule{
keeper: keeper,
}
Expand Down
12 changes: 12 additions & 0 deletions testutil/x/counter/types/expected_keepers.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package types

import "context"

type CounterHooks interface {
AfterIncreaseCount(ctx context.Context, newCount int64) error
}

type CounterHooksWrapper struct{ CounterHooks }

// IsOnePerModuleType implements the depinject.OnePerModuleType interface.
func (CounterHooksWrapper) IsOnePerModuleType() {}
26 changes: 26 additions & 0 deletions testutil/x/counter/types/hooks.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package types

import (
"context"
"errors"
)

var _ CounterHooks = MultiCounterHooks{}

// MultiCounterHooks is a slice of hooks to be called in sequence.
type MultiCounterHooks []CounterHooks

// NewMultiCounterHooks returns a MultiCounterHooks from a list of CounterHooks
func NewMultiCounterHooks(hooks ...CounterHooks) MultiCounterHooks {
return hooks
}

// AfterIncreaseCount calls AfterIncreaseCount on all hooks and collects the errors if any.
func (ch MultiCounterHooks) AfterIncreaseCount(ctx context.Context, newCount int64) error {
var errs error
for i := range ch {
errs = errors.Join(errs, ch[i].AfterIncreaseCount(ctx, newCount))
}

return errs
}
Loading