Allow depinject interface types to be resolved using a key

[aaronc]

Problem Definition

Currently depinject dependencies must be exact matches so the expected keepers paradigm will no longer work.

For example, say the bank module provides bank.Keeper and the nft module wants to require nft.ExpectedBankKeeper which is a sub-interface of bank.Keeper. Depinject can't resolve this dependency. In #11908, we propose that the keeper should suggest bank.Keeper as a hint.

We could extend this and do a linear search of all interfaces in the container to see if there is a match, but introducing a linear search into dependency resolution is probably unacceptable even if this is only done at app initialization.

Propose Solution

As an alternative, we propose an optional key parameter which can be provided when using depinject and depinject structs.

For example, the bank module could provide:

type outputs struct {
  depinject.Out

  BankKeeper keeper.Keeper `key:"cosmos.bank.module.v1.Keeper"`
}

and the NFT module could require:

type inputs struct {
  depinject.In
  
  BankKeeper nft.ExpectedBankKeeper `key:"cosmos.bank.module.v1.Keeper"`
}

Modules would need to coordinate on the correct key names which are just strings chosen to be logical and unambiguous, but this is probably preferable to doing a linear search. It may also allow a way to provide different dependencies to different modules if needed (see #11911)

[kocubinski]

Haven't fully thought this through, but an alternative implementation could look like

bank:

type outputs struct {
  depinject.Out

  BankKeeper keeper.Keeper `bindings:"cosmos.nft.module.v1.ExpctedBankKeeper,cosmos.staking.module.v1.ExpectedBankKeeper"`
}

nft:

type inputs struct {
  depinject.In
  
  BankKeeper nft.ExpectedBankKeeper
}

[aaronc]

But what we want to avoid is bank needing to have knowledge of nft, staking, etc.

[aaronc]

The linear search option could also be valid, but we'd need a way to handle conflicts still. That could either be via a key and/or the approach described in #11911

[kocubinski]

Ok true. I'm used to seeing those kind of bindings described somewhere else, a composition root outside of the modules themselves.

[aaronc]

Here are the designs I've thought of that we could choose between at the depinject layer:

1. match interfaces based on a linear search and bind them if there's no conflict - this is probably the simplest for users but also slow and not-explicit
2. match interfaces which are compatible based on keys - this has the downside of needing to coordinate on string keys
3. have a Prefer config option which allows the container to resolve conflicts in case there are two matches either for an exact type, interfaces matched using (1) linear search, or (2) key matches. there could also be PreferInModule option to choose different conflict resolution for different modules

If we did just (1), I think (3) is necessary. If we did (2), we could postpone or even skip (3)

Any other solutions anyone can think of? Any strong preferences on which strategy?

[aaronc]

Also note that we should probably do #11908 while we're doing this and that will help the dev UX

[kocubinski]

(2) wasn't too bad of patch in #12103. I'm still interested in (1) as a fallback, though.

[aaronc]

Either way we should do ##11908 first

[aaronc]

Now that we're working with keys do we like this approach or should we also implement (1)? The immediate downside of (2) is what if the provider fails to use key, then the consumer is stuck. (1) together with (3) would alleviate that.

Thoughts @kocubinski @facundomedica @blushi @JeancarloBarrios now that we're using this?

[kocubinski]

There are some concerns around how (1) will work when there are potentially multiple implementations of an interface. This could happen if 2 or more separate provider functions in different modules have single interface implementations as outputs. @marbar3778 mentioned an Interchain security package is currently in this state. However I still like this feature from a usability perspective even if it doesn't address all cases; (1) with (3) will let the user know that depinject needs more specific instructions to build the application's dependency tree.

The downsides of (2) still stand however. Possible remediation:

• Tag inputs/outputs somehow from app.yaml instead of next to the module provider code. Removes the need to change a providing module.
• Express bindings in app.yaml explicitly, e.g. github.com/cosmos/cosmos-sdk/x/bank/types/AccountKeeper -> github.com/cosmos/cosmos-sdk/x/auth/keeper#AccountKeeper

[aaronc]

I'm thinking we should maybe take an approach where we don't auto-magically bind things as described in (1), but that an explicit app.yaml binding like you've described github.com/cosmos/cosmos-sdk/x/bank/types/AccountKeeper -> github.com/cosmos/cosmos-sdk/x/auth/keeper#AccountKeeper is needed

[JeancarloBarrios]

I like the idea of being quite explicit and expressing the binding in the app.yaml like @kocubinski mentions is the one I feel more comfortable with.

[aaronc]

From the SDK call, sounds like we're no going with (1) and (3).

So we'd have automatic matching of interfaces if there is no conflict and conflict resolution if there is (depinject/appconfig should give you suggestions on how to address).

At the depinject level, I think we want to add two config options:

func Prefer(inTypeName, outTypeName string) Config { ... }

func PreferInModule(moduleName, inTypeName, outTypeName string) Config { ... }

and then in the app.yaml, we could do something like this:

modules:
  - name: bank
    config: ...
    golang_bindings:
     github.com/cosmos/cosmos-sdk/x/bank/types.AccountKeeper: github.com/cosmos/cosmos-sdk/x/auth/keeper.AccountKeeper

golang_bindings could exist at both the top-level and module level (PreferInModule) although module-level would probably be more common.

The error from depinject should be strongly typed so that appconfig can give a suggestion based on the yaml config (remember that depinject knows nothing about yaml appconfig).

Does this approach sound right?

[facundomedica]

I think we can close this now that #12169 is merged. Please reopen if you think otherwise

[aaronc]

This isn't supported in the app config yet so we should reopen

[aaronc]

@kocubinski we also need to make sure explicit and implicit interface bindings are properly reflected in the graphviz output. Can you take a look at that after #12367 ?