Bug: dependency injection and bank hooks for token factory

[faddat]

Summary

When using depinject and an ignite base, hooks that we set are getting nilled out, even though they do get successfully set.

Neutron

• gov gated bank hooks
• implementation otherwise the same as osmosis

Neutron App

https://github.com/neutron-org/neutron/blob/fba0200a0d2353fffe0c143a24cfd84bf0d23a38/app/app.go#L707-L721

Neutron Cosmos-sdk fork

• https://github.com/neutron-org/cosmos-sdk/tree/neutron/x/bank

Osmosis

• bank hooks free for anyone to use
• implementation the same except Osmosis has better variable names in the sdk

Osmosis App

https://github.com/osmosis-labs/osmosis/blob/09ff5a2d72306f5b5a4b4dd29996cb5ab37136d7/app/keepers/keepers.go#L836-L847

Osmosis Cosmos-sdk fork

https://github.com/osmosis-labs/cosmos-sdk/tree/osmo/v0.50.x/x/bank

us

We've tried taking the changes to x/bank (only those related to these hooks, as osmosis has some additional changes to bank) and putting them in our cosmos-sdk fork, which contains no other code changes. We've tried both the osmo style and the ntrn style, and we get the same result, where hooks are successfully set and then nilled later.

The issue is that we've tried three ways of integrating the bank hooks to the token factory, and in each case:

• bank hooks are successfully set
• bank hooks are nilled out later

we've confirmed this with a debugger, and have tried using the exact contract code on the Osmosis testnet. the contract code works on Osmosis testnet, but doesn't in our scenario, specifically, after freezing, it is still possible to send funds.

@julienrbrt has provided this:

• docs: demonstrate how to define hooks using depinject cosmos/cosmos-sdk#21892

and we are trying that. Here are code samples of our implementations, which ought to work:

In one case, we made a shim named tokenfactory.go -- like the shims currently used for wasm.go and ibc.go in ignite:

func (app *App) registerTokenFactoryModule(
	appCodec codec.Codec,
) {
	// Initialize the token factory keeper
	tokenFactoryKeeper := tokenfactorykeeper.NewKeeper(
		appCodec,
		runtime.NewKVStoreService(app.GetKey(tokenfactorytypes.StoreKey)),
		knownModules(),
		app.AccountKeeper,
		app.BankKeeper,
		app.WasmKeeper,
		authtypes.NewModuleAddress(govtypes.ModuleName).String(),
	)
	app.TokenFactoryKeeper = tokenFactoryKeeper

	// Set the hooks on the BankKeeper
	app.BankKeeper.SetHooks(
		banktypes.NewMultiBankHooks(
			app.TokenFactoryKeeper.Hooks(),
		),
	)

	// Add TokenFactoryKeeper to the app's module manager
	app.ModuleManager.Modules[tokenfactorytypes.ModuleName] = tokenfactory.NewAppModule(appCodec, app.TokenFactoryKeeper)
}

In two other cases that are basically the same as each other, we integrated in app.go, only changing the order slightly:

app.go number one

	app.BankKeeper.BaseSendKeeper = app.BankKeeper.BaseSendKeeper.SetHooks(
		banktypes.NewMultiBankHooks(
			app.TokenFactoryKeeper.Hooks(),
		))

	app.TokenFactoryKeeper.SetContractKeeper(app.WasmKeeper)

app.go number two

   	app.TokenFactoryKeeper.SetContractKeeper(app.WasmKeeper)


	app.BankKeeper.BaseSendKeeper = app.BankKeeper.BaseSendKeeper.SetHooks(
		banktypes.NewMultiBankHooks(
			app.TokenFactoryKeeper.Hooks(),
		))

results of scenarios

In every case, the hooks were set successfully, verifying by debugger. The trouble is that they also got unset, and we do not know why, other than that the key difference between Osmosis, Neutron, and what we are cooking is that we are using dependency injection.

My understanding from speaking with Julien is that the routes we attempted ought to work -- and that was my understanding as well.

When we've put together something that resembles what is suggested in the SDK documentation pr, we will update this issue again, with the note that the goal with dependency injection was to allow for shims to work properly, when needed.

The issue for ignite users here is that it seems strongly possible that there is some kind of issue in depinject that can cause unpredictable behavior.

The modules in question

Token factory

we want to use the the hooks in bank from tokenfactory. It is important to note that tokenfactory interacts with x/wasm Our tokenfactory module's wiring section looks like:

func init() {
	appmodule.Register(
		&modulev1.Module{},
		appmodule.Provide(ProvideModule),
	)
}

type ModuleInputs struct {
	depinject.In

	Cdc          codec.Codec
	StoreService store.KVStoreService
	Config       *modulev1.Module
	Logger       log.Logger

	AccountKeeper types.AccountKeeper
	BankKeeper    types.BankKeeper
}

type ModuleOutputs struct {
	depinject.Out

	CoinfactoryKeeper keeper.Keeper
	Module            appmodule.AppModule
}

func ProvideModule(in ModuleInputs) ModuleOutputs {
	// default to governance authority if not provided
	authority := authtypes.NewModuleAddress(govtypes.ModuleName)
	if in.Config.Authority != "" {
		authority = authtypes.NewModuleAddressOrBech32Address(in.Config.Authority)
	}
	k := keeper.NewKeeper(
		in.Cdc,
		in.StoreService,
		in.Config.KnownModules,
		in.AccountKeeper,
		in.BankKeeper,
		nil,
		authority.String(),
	)
	m := NewAppModule(
		in.Cdc,
		k,
	)

	return ModuleOutputs{CoinfactoryKeeper: k, Module: m}
}

x/wasm

There is no app wiring section in x/wasm at all:

• https://github.com/CosmWasm/wasmd/blob/v0.53.0/x/wasm/module.go
  so apparently they aren't just waiting on IBC, but instead not attempting to support the app wiring scheme at all.

bank

Using the Osmosis bank module's module.go file:

https://github.com/osmosis-labs/cosmos-sdk/blob/e7668cab6be057191ee826fee2dee9d0dc2caab7/x/bank/module.go#L196-L296

// App Wiring Setup

func init() {
	appmodule.Register(
		&modulev1.Module{},
		appmodule.Provide(ProvideModule),
		appmodule.Invoke(InvokeSetSendRestrictions),
	)
}

type ModuleInputs struct {
	depinject.In

	Config       *modulev1.Module
	Cdc          codec.Codec
	StoreService corestore.KVStoreService
	Logger       log.Logger

	AccountKeeper types.AccountKeeper

	// LegacySubspace is used solely for migration of x/params managed parameters
	LegacySubspace exported.Subspace `optional:"true"`
}

type ModuleOutputs struct {
	depinject.Out

	BankKeeper keeper.BaseKeeper
	Module     appmodule.AppModule
}

func ProvideModule(in ModuleInputs) ModuleOutputs {
	// Configure blocked module accounts.
	//
	// Default behavior for blockedAddresses is to regard any module mentioned in
	// AccountKeeper's module account permissions as blocked.
	blockedAddresses := make(map[string]bool)
	if len(in.Config.BlockedModuleAccountsOverride) > 0 {
		for _, moduleName := range in.Config.BlockedModuleAccountsOverride {
			blockedAddresses[authtypes.NewModuleAddress(moduleName).String()] = true
		}
	} else {
		for _, permission := range in.AccountKeeper.GetModulePermissions() {
			blockedAddresses[permission.GetAddress().String()] = true
		}
	}

	// default to governance authority if not provided
	authority := authtypes.NewModuleAddress(govtypes.ModuleName)
	if in.Config.Authority != "" {
		authority = authtypes.NewModuleAddressOrBech32Address(in.Config.Authority)
	}

	bankKeeper := keeper.NewBaseKeeper(
		in.Cdc,
		in.StoreService,
		in.AccountKeeper,
		blockedAddresses,
		authority.String(),
		in.Logger,
	)
	m := NewAppModule(in.Cdc, bankKeeper, in.AccountKeeper, in.LegacySubspace)

	return ModuleOutputs{BankKeeper: bankKeeper, Module: m}
}

func InvokeSetSendRestrictions(
	config *modulev1.Module,
	keeper keeper.BaseKeeper,
	restrictions map[string]types.SendRestrictionFn,
) error {
	if config == nil {
		return nil
	}

	modules := maps.Keys(restrictions)
	order := config.RestrictionsOrder
	if len(order) == 0 {
		order = modules
		sort.Strings(order)
	}

	if len(order) != len(modules) {
		return fmt.Errorf("len(restrictions order: %v) != len(restriction modules: %v)", order, modules)
	}

	if len(modules) == 0 {
		return nil
	}

	for _, module := range order {
		restriction, ok := restrictions[module]
		if !ok {
			return fmt.Errorf("can't find send restriction for module %s", module)
		}

		keeper.AppendSendRestriction(restriction)
	}

	return nil
}

Larger picture

Complex existing codebases need to move to dependency injection in order to use cosmos-sdk v0.52.x

....I don't know if they will be able to do this safely or successfully.

Taking the existing style from Neutron or Osmosis ought to work without issue, but ultimately doesn't.

Thanks to Ignite

Super appreciative of Julien's rapid response to this issue.

[julienrbrt]

Hey, happy to hop on a call to discuss that. When are you available?

[faddat]

right now, if you are. I'll send a DM on X so we can coordinate.

[faddat]

@julienrbrt I moved all the detail into the head of this issue, and thanks again for reaching out. Super appreciated.

[julienrbrt]

We discussed a bunch on X. The conclusion was Osmosis and Neutron fork of bank haven't wired the hooks to be depinject compatible. They needed to add an invoker that will call SetHooks afterwards. Modules can then output their bank hooks (such as token factory for instance).

The other solution, is it set it manually by calling SetHooks on the bank keeper after runtime.Build. It would be preferable however if they properly wired their bank hooks in their bank fork.

Feel free to re-open if I missed something.

[faddat]

I can't reopen, I do not have that option on this repository.

This is a product issue, and should likely remain open as people might want to use hooks. Ignite doesn't tell them that they can't. And there are three modules involved. I've outlined above, and I've found, with a single tweet, others who are using DI and having a bad time of it.

So let's get to the bottom of it.

• Someone walking up to ignite for the first time would have no idea about any of this
• they would only know that the shims (like wasm.go and ibc.go) don't actually allow for integration in the way described, then they would become sad
• the descriptions given, actually say that using a shim, will work

thus, the issue should remain open, and/or move to the cosmos-sdk

Should only be closed if/when there is not an enormous and serious footgun / inability to use features that are promised to exist in depinject including the ability to wire modules manually.

As we discussed, that doesn't exist.

[julienrbrt]

Re-opening, but as said above, it's Osmosis and Neutron bank forks that do not support hooks with depinject.
This doesn't fall on the SDK team or Ignite. We are indeed providing more documentation about how to do it (cosmos/cosmos-sdk#21892).
SDK modules such as staking or gov, support hooks and are fully depinject compatible

[faddat]

I'm gonna make sure that they do, but I think the issue here may actually be in x/wasm