change type of scopedKeeper in all Keepers to interface

[crodriguezvega]

The scopedKeeper in all Keepers in ibc-go has at the moment a concrete type (i.e. capabilitykeeper.ScopedKeeper), but there is no good reason why we should not use an interface instead. Other keepers in the Keeper types are already using interfaces.

Proposal

1. Add ScopedKeeper interface.

For 05-port create an expected_keepers.go file in package types and add the following interface:

type ScopedKeeper interface {
  NewCapability(ctx sdk.Context, name string) (*capabilitytypes.Capability, error)
  GetCapability(ctx sdk.Context, name string) (*capabilitytypes.Capability, bool)
  AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) bool
  LookupModules(ctx sdk.Context, name string) ([]string, *capabilitytypes.Capability, error)
}

For 04-channel add the following interface to expected_keepers.go:

type ScopedKeeper interface {
  NewCapability(ctx sdk.Context, name string) (*capabilitytypes.Capability, error)
  AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) bool
  LookupModules(ctx sdk.Context, name string) ([]string, *capabilitytypes.Capability, error)
}

For transfer add the following interface to expected_keepers.go:

type ScopedKeeper interface {
  GetCapability(ctx sdk.Context, name string) (*capabilitytypes.Capability, bool)
  AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) bool
  LookupModules(ctx sdk.Context, name string) ([]string, *capabilitytypes.Capability, error)
}

For 27-interchain-accounts add the following interface to expected_keepers.go:

type ScopedKeeper interface {
  GetCapability(ctx sdk.Context, name string) (*capabilitytypes.Capability, bool)
  AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) bool
  LookupModules(ctx sdk.Context, name string) ([]string, *capabilitytypes.Capability, error)
}

where capabilitytypes is alias for github.com/cosmos/cosmos-sdk/x/capability/types.

2. Change the type for the scopedKeeper field in the Keeper type of:

• 05-port - PR
• 04-channel - PR
• transfer - PR
• 27-interchain-accounts controller and 27-interchain-accounts host - PR

to the corresponding interface type created on step 1.

Notes

• Consider opening different PRs for the changes affecting each part of the codebase.
• This is an API breaking change.

[Anmol1696]

Would like to take this up?

[crodriguezvega]

Would like to take this up?

That would be great! Thanks! I will assign the issue to you now. Don't hesitate to reach out if there's something we can help you with.

[Anmol1696]

Wanted to ask, how is this API breaking change? Since the scopedKeeper is just an unexported struct field.

[Anmol1696]

Created the first PR for 04-channel, let me know if there is anything missing here, will be creating following PRs.
The only thing i am still not sure off is the changes needed for the core keeper.

[crodriguezvega]

Wanted to ask, how is this API breaking change? Since the scopedKeeper is just an unexported struct field.

Thanks for asking! We are changing the type of the field, so the NewKepper function accepts now as well a different type for this field, and since this function is part of our external API, then we consider it a breaking change. Basically what @colin-axner also says here. I hope this answers your question!

[Anmol1696]

Yup it does, thanks. Will complete all the other remaining PRs.

[Anmol1696]

After all the PRs, I feel the only place left where we do not use a keeper interface is core/keeper.

Do you guys think it makes sense to have a ScopedKeeper interface? Would a bit tricky since we would have to create interface with union of methods in core/types perhaps or use the interface from one of the modules which has the most methods, like the way this NewKeeper uses clienttypes.StakingKeeper interface.

[colin-axner]

You can add the interface to the exported directory. Maybe add a expected_keepers.go file?

[crodriguezvega]

@Anmol1696 would you like to try @colin-axner's proposal to see if that would work?

[damiannolan]

You can add the interface to the exported directory. Maybe add a expected_keepers.go file?

Would this interface fit in channel.go? 🤔

[Anmol1696]

Sorry for the late response on this, missed the notifications.

What would be the benifit of adding the interface to core/exported package? The other interfaces of various keepers in <some module>/types/expected_keepers.go are not in the exported package. ScopedKeeper is kind of a keeper.

Maybe I am missing the point of the exported package itself.

[colin-axner]

The purpose of the exported directory is to expose interfaces which will be imported outside of a single package which allows us to work around import cycles created by the dependency of core/types on its subdirectory types. It allows us to import shared interfaces. Putting the interface in the 04-channel types directory is probably more intuitive. 05-port already imports channeltypes anyways (which is the other submodule using this keeper)

[Anmol1696]

So the expection here is we move all the ScopedKeeper interface to exported package and use that instead of defining the similar interface everywhere (04-channel, 05-port, transfer, 27-interchain)?

[Anmol1696]

I can restrcuture the interfaces if you think so, just want to understand that is what is needed here.

[colin-axner]

I don't have a strong preference. I'd be fine leaving the scoped keeper as a concrete type in the ibc keeper, there isn't a good reason for it to be swapped for a different capability implementation. If we wanted to make it an interface, I think all scoped keepers relying on an interface within exported probably makes the most sense

What do you think @Anmol1696? Should we make the change or close the issue?

cc @damiannolan

[damiannolan]

Personally I feel like this would have been fine left as the concrete type from x/capability. Like you said I don't see any reason why someone would use a modified capability implementation. But given that the current state is a ScopedKeeper interface in each module I would at least like to see it move out to exported so that it is common.

[Anmol1696]

The only reason to make this into an interface in the first place was to add composibility for each of the package. Moving it to exported to me makes it a weird dependency loop. Either i am not understanding the name of the package exported, to me it is still weird internal packages use exported interfaces.

But since both @damiannolan and @colin-axner both want the interface to be moved to exported package, i will create the PR for the same then.

[Anmol1696]

For the PR I moved the scoped interface to core/exported package, and that is used everywhere now.
I am not 100% sure on the dependency loop it creates from app to core though.

Let me know if it makes sense, or we want to keep the app ScopedInterface seperate.

PS. I also updated the changelog, but all points point to the same PR. Let me know if that is ok.

[colin-axner]

I am not sure I follow your concerns. exported perhaps isn't the best naming, but the purpose of the exported package is to gather all the shared interfaces into one package. The exported package should not import any of ibc-go (thus creating import cycles). Does that ease your concerns?

[Anmol1696]

Makes sense @colin-axner thanks. Yes, not a concern, just the name exported confused me.