faddat commented 1 month ago

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 commented 1 month ago

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

faddat commented 1 month ago

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

faddat commented 1 month ago

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

julienrbrt commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

faddat commented 1 month ago

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

ignite / cli

FIX: Dependency Injection and Bank Hooks for Token Factory #4379

Summary

Neutron

Osmosis

us

app.go number one

app.go number two

results of scenarios

The modules in question

Token factory

x/wasm

bank

Larger picture

Thanks to Ignite