Push Chain.SendMsgs in ibctesting to instead be an interface or passed in function

[ValarDragon]

Summary

In Osmosis, governance has decided it needs to have a consensus minimum fee. An issue with doing so comes from ibctesting! (Txs failing due to 0 fee) Working on a quick hack to avoid this (at expense of test quality), but right now ibctesting assumes it can handle the []sdk.Msg -> Tx conversion entirely on its own. This is done in this method: https://github.com/cosmos/ibc-go/blob/main/testing/chain.go#L313-L347 .

SendMsgs is called in coordinator.Setup(suite.path), so we can't really avoid this call happening. And its sensible for ibctesting to generally just need this method to work, in order to make the guarantees it needs to. I also suspect theres more guarantees / areas the caller could want to test (e.g. on a Network with no Secp keys, or who wants to have default path ensure threshold encryption accuracy)

Proposal

I suggest we allow TestChain to have as a field, which we can set, to be an overrideable method to call for SendMsgs.

So this would look like:

type TestChain struct {
... // stuff
SendMsgOverride func(sdk.Msg...) (sdk.Result, error)
}

func (t TestChain) SendMsgs(msgs sdk.Msg...) (sdk.Result, error) {
  if t.SendMsgOverride != nil {
    return t.SendMsgOverride(msgs)
  }
  // Do Old stuff
}

An alternate option would be to make chain an interface (which is likely long term better), but that seems like a higher code lift.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[colin-axner]

Thanks @ValarDragon. For my understanding, the issue is this function as it generates empty fee transaction?

I've found this function problematic for other reasons, such as expPass being hard coded to true. I'm okay with the proposed change. Seems like a decent tradeoff between flexibility for these sorts of issues while still providing a default behaviour for developers who don't need the flexibility. I think the chain interface would be the same as adding this function to the TestingApp interface? I think it's better to do the simple fix and move on, it's hard to tell the exact future of the testing pkg design, I could see it changing a lot going forward

[ValarDragon]

Yes, that function is the problem! Agreed that that function is otherwise problematic.

Ah didn't realize there was already the testingApp interface, its equivalent there then. Happy with the simple fix then!

[colin-axner]

We discussed as a team a while back and agreed to go with this solution for now (keep it simple). We should document that this is a temporary hack and indicate why it is useful