Cosmos-sdk: Modules should use local module interfaces instead of types/

[sunnya97]

[ebuchman]

I wonder if we should try to roll this out more generally. Eg. maybe bank doesn't need to import auth, it can just define a local AccountMapper interface... etc.

[rigelrozanski]

Yeah this was the idea - each module should define what it needs from an interface locally and then the application can pass in the relevant objects which fulfill the receiving modules needs.

[jaekwon]

Sounds like a good general principle. OTOH if it's something that takes a long time to refactor but can be done later after launch without making changes to the underlying store spec, then we should consider tabling it until the next release of the SDK.

[ValarDragon]

@sunnya97 can we close this? I think the aspects of staking governance requires is in types.

[rigelrozanski]

@ValarDragon I don't think we should close this, based on this and offline conversations, it seems like a good idea to move receiving-keeper interfaces from types/ into the receiver's x/ folder. But I agree with Jae this can be post-launch no problem

[sunnya97]

Yeah, seconding to what Rigel said. And agreed, this can be post-launch.

[rigelrozanski]

This principle also applies to module hooks

[jackzampolin]

Can we now close this?

[rigelrozanski]

leave open - we haven't implemented yet haha - (although I have started using this design pattern for distribution)

[sunnya97]

I think we should have some sort of SDK module best practices guide.

[rigelrozanski]

Yeah I mean currently we have coding standards in a tendermint repo - may be good to move this right into the SDK

[hleb-albau]

Do you plan to make account keeper as interface in near future? In our model, we need redefine some methods, but its not possible on struct.

[alexanderbez]

Yes. We have no intentions of it not being an interface. What are the problems you're running into?

[hleb-albau]

@alexanderbez we need to add some hooks for coins transfer. Best way is to just wrap default AccKeeper and add addition logic. But others modules should expect interface but not struct.

[alexanderbez]

Ahhh wait, the account keeper is not an interface. I guess perhaps it should be! Although, I'd be weary of using hooks. Our use of hooks is not necessarily the best approach and already has shown major drawbacks.

[hleb-albau]

@alexanderbez yeah, I just mean some custom logic should be done in our case, that why we need to extend default AK. But it is not an interface....
As I read discussion, you plan to refactor Ak only after launch. Maybe it can be done by multiple small steps? If so, we can try to contribute here.

[alexanderbez]

Yeah feel free to open a PR making the keeper an interface. We recently did this with the bank keeper.

[hleb-albau]

@alexanderbez By the way, what is better than hooks? How to implement same logic, but without hook?

[jackzampolin]

Sounds like we need to have a discussion, and develop a spec, about how to pass cross module events. Thoughts @alexanderbez @cwgoes?

[cwgoes]

Hooks can also be defined in receiving modules @jackzampolin, so this issue would apply.

How to implement same logic, but without hook?

I don't understand this question. Which part of the "same" logic do you want to implement? A hook is just a way for one module to call into another.

[hleb-albau]

Although, I'd be weary of using hooks. Our use of hooks is not necessarily the best approach and already has shown major drawbacks.

I don't understand this question. Which part of the "same" logic do you want to implement? A hook is just a way for one module to call into another.

Just interested in better way(better then hooks) of inter-module communications.

[jackzampolin]

Going to close this issue as we have added the expected keeper abstraction. Please reopen if I'm wrong.

[rigelrozanski]

this will be closed by fede's PR https://github.com/cosmos/cosmos-sdk/pull/4417