From de215da79ade5e24322804fc0e2cdd9b58a8974f Mon Sep 17 00:00:00 2001
From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com>
Date: Mon, 8 Jan 2024 12:01:13 +0100
Subject: [PATCH] Improving docstrings for UpgradableModule interface (#5474)
 (#5529)

* chore: merge main

* chore: corrected commit

* Document ibc app expectations and recommendations (#5506)

* fix typo

---------

Co-authored-by: Charly <charly@interchain.io>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
(cherry picked from commit 26fd1e0adead64f4e432d1ce33216c201852f163)

Co-authored-by: Cian Hatton <cian@interchain.io>
---
 docs/docs/01-ibc/06-channel-upgrades.md | 18 ++++++++++++++++++
 modules/core/05-port/types/module.go    | 23 ++++++++++++++++++-----
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/docs/docs/01-ibc/06-channel-upgrades.md b/docs/docs/01-ibc/06-channel-upgrades.md
index 6bc445b2bc7..69df5b18f44 100644
--- a/docs/docs/01-ibc/06-channel-upgrades.md
+++ b/docs/docs/01-ibc/06-channel-upgrades.md
@@ -90,3 +90,21 @@ the channel will move back to `OPEN` state keeping its original parameters.
 The application's `OnChanUpgradeRestore` callback method will be invoked.
 
 It will then be possible to re-initiate an upgrade by sending a `MsgChannelOpenInit` message.
+
+## IBC App Recommendations
+
+IBC application callbacks should be primarily used to validate data fields and do compatibility checks.
+
+`OnChanUpgradeInit` should validate the proposed version, order, and connection hops, and should return the application version to upgrade to.
+
+`OnChanUpgradeTry` should validate the proposed version (provided by the counterparty), order, and connection hops. The desired upgrade version should be returned.
+
+`OnChanUpgradeAck` should validate the version proposed by the counterparty.
+
+`OnChanUpgradeOpen` should perform any logic associated with changing of the channel fields.
+
+`OnChanUpgradeRestore`  should perform any logic that needs to be executed when an upgrade attempt fails as is reverted.
+
+> IBC applications should not attempt to process any packet data under the new conditions until after `OnChanUpgradeOpen`
+> has been executed, as up until this point it is still possible for the upgrade handshake to fail and for the channel
+> to remain in the pre-upgraded state. 
diff --git a/modules/core/05-port/types/module.go b/modules/core/05-port/types/module.go
index a5c6386f926..23dba849307 100644
--- a/modules/core/05-port/types/module.go
+++ b/modules/core/05-port/types/module.go
@@ -107,8 +107,12 @@ type IBCModule interface {
 }
 
 // UpgradableModule defines the callbacks required to perform a channel upgrade.
+// Note: applications must ensure that state related to packet processing remains unmodified until the OnChanUpgradeOpen callback is executed.
+// This guarantees that in-flight packets are correctly flushed using the existing channel parameters.
 type UpgradableModule interface {
-	// OnChanUpgradeInit initializes the channel upgrade handshake.
+	// OnChanUpgradeInit enables additional custom logic to be executed when the channel upgrade is initialized.
+	// It must validate the proposed version, order, and connection hops.
+	// Note: in the case of crossing hellos, this callback may be executed on both chains.
 	OnChanUpgradeInit(
 		ctx sdk.Context,
 		portID, channelID string,
@@ -117,7 +121,9 @@ type UpgradableModule interface {
 		version string,
 	) (string, error)
 
-	// OnChanUpgradeTry verifies the counterparty upgrade and sets the upgrade on TRY chain
+	// OnChanUpgradeTry enables additional custom logic to be executed in the ChannelUpgradeTry step of the
+	// channel upgrade handshake. It must validate the proposed version (provided by the counterparty), order,
+	// and connection hops.
 	OnChanUpgradeTry(
 		ctx sdk.Context,
 		portID, channelID string,
@@ -126,7 +132,8 @@ type UpgradableModule interface {
 		counterpartyVersion string,
 	) (string, error)
 
-	// OnChanUpgradeAck TODO
+	// OnChanUpgradeAck enables additional custom logic to be executed in the ChannelUpgradeAck step of the
+	// channel upgrade handshake. It must validate the version proposed by the counterparty.
 	OnChanUpgradeAck(
 		ctx sdk.Context,
 		portID,
@@ -134,7 +141,9 @@ type UpgradableModule interface {
 		counterpartyVersion string,
 	) error
 
-	// OnChanUpgradeOpen TODO
+	// OnChanUpgradeOpen enables additional custom logic to be executed when the channel upgrade has successfully completed, and the channel
+	// has returned to the OPEN state. Any logic associated with changing of the channel fields should be performed
+	// in this callback.
 	OnChanUpgradeOpen(
 		ctx sdk.Context,
 		portID,
@@ -144,7 +153,11 @@ type UpgradableModule interface {
 		version string,
 	)
 
-	// OnChanUpgradeRestore TODO
+	// OnChanUpgradeRestore enables additional custom logic to be executed when any of the following occur:
+	//    - the channel upgrade is aborted.
+	//    - the channel upgrade is cancelled.
+	//    - the channel upgrade times out.
+	// Any logic set due to an upgrade attempt should be reverted in this callback.
 	OnChanUpgradeRestore(
 		ctx sdk.Context,
 		portID,