cli
cli copied to clipboard
ignite
Bug: dependency injection and bank hooks for token factory
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:
- https://github.com/cosmos/cosmos-sdk/pull/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 I moved all the detail into the head of this issue, and thanks again for reaching out. Super appreciated.
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.
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.
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 (https://github.com/cosmos/cosmos-sdk/pull/21892). SDK modules such as staking or gov, support hooks and are fully depinject compatible
I'm gonna make sure that they do, but I think the issue here may actually be in x/wasm
Closing here, as it was indeed an issue in hooks setup, and not with depinject. This is further explained here: https://docs.cosmos.network/v0.52/build/building-modules/define-hooks