Implement 29-fee channel upgrade callbacks

[damiannolan]

Summary

With the addition #1831, the 04-channel upgrade callbacks will be added to the IBCModule interface.
This enforces applications to implement the required callbacks.

The 29-fee module must handle the upgrade callbacks appropriately and follow similar logic to the channel opening handshake:

• Attempt to unmarshal the provided version string to types.Metadata.
• In the event of failure it should pass through to the underlying app.
• The provided FeeVersion must be compatible with the app version: FeeVersion == types.Version
• The OnChanUpgradeInit and OnChanUpgradeTry callbacks should handle the underlying app version and set it in the JSON encoded types.Metadata string returned.
• Set the channel as fee enabled.

---

For Admin Use

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

[damiannolan]

Needs #4125 for OnChanUpgradeRestore

[colin-axner]

I think we got confused implementing this issue since it was written in 2022 and didn't account for the latest designs. The fee enablement change should only occur upon a successful completion of the upgrade handshake. In that case OnChanUpgradeRestore should be a no-op. Given the latest spec changes, our handlers should perform the following functionality:

onChanUpgradeInit validates the upgrade fields, optionally returns modified version
onChanUpgradeTry validates the upgrade fields, optionally returns modified version
onChanUpgradeAck validates the upgrade version
onChanUpgradeOpen upgrade to new fields (set fee enable)
onChanUpgradeRestore should no-op

I would recommend opening a pr for each to readjust

[damiannolan]

I agree, that given the new design structure (upgrade fields are not taking effect until ChanUpgradeOpen) that it probably makes more sense to not make application level changes until the onOpen callback.
But we can't make any guarantees that other applications won't do what they want on each callback handler.

With the current API for callbacks I don't think we can actually make the above changes work (we're not provided the version):

OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string)

We get the port/channel from ibc core and that is it. I know we already had some discussions about changing these APIs, I'm not sure what would actually work the best for applications. But something like below might work better, it would also provide a more forwards compatible API if new fields are added for upgradability.

OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, upgrade channeltypes.UpgradeFields) (string, error)

OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, upgrade channeltypes.UpgradeFields) (string, error)

// ...

OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, upgrade channeltypes.UpgradeFields)

Any thoughts?

[chatton]

With our current design of OnChanUpgradeInit / Try etc. we are explicitly passing in each of the fields from the upgrade. One of the reasons to do this was so that adding new fields is api breaking to not make it so that the handlers can accidentally ignore a new field.

[colin-axner]

OnChanUpgradeOpen should be taking each upgrade field. It's unsafe for middlewares to change packet processing behaviour before an upgrade completes. I agree it's hard to enforce validation only for the first 3 steps

[damiannolan]

It's unsafe for middlewares to change packet processing behaviour before an upgrade completes.

Very true! If packets still need to be processed while in FLUSHING then middlewares should not be doing anything extra all of a sudden. I think it will be more clear when states are FLUSHING/FLUSHED that packets still must be processed on the channel under the same conditions - this should be well documented tho!