feat: adding OnChanUpgradeInit handler implementation to 29-fee (#…

…4019) * WIP: adding fee upgrade cbs and testing * imp: allow failure expectations when using chain.SendMsgs * fixing import errors from cherry-pick * updating tests and rm try code * rm diff onChanUpgradeTry * Update modules/apps/29-fee/ibc_middleware.go * adding MetadataFromVersion func to pkg types * addressing pr feedback, disable fees on err, rename args, adding testcase * Update modules/apps/29-fee/ibc_middleware_test.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * abstract out expIsFeeEnabled check in tests * adding additional error context to MetadataFromVersion --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
cosmos · Jul 17, 2023 · f731de5 · f731de5
1 parent 8e53c77
commit f731de5
Show file tree

Hide file tree

Showing 6 changed files with 199 additions and 20 deletions.
diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go
@@ -321,8 +321,43 @@ func (im IBCMiddleware) OnTimeoutPacket(
 }
 
 // OnChanUpgradeInit implements the IBCModule interface
-func (im IBCMiddleware) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, sequence uint64, version, previousVersion string) (string, error) {
-	return im.app.OnChanUpgradeInit(ctx, portID, channelID, order, connectionHops, sequence, version, previousVersion)
+func (im IBCMiddleware) OnChanUpgradeInit(
+	ctx sdk.Context,
+	portID string,
+	channelID string,
+	order channeltypes.Order,
+	connectionHops []string,
+	upgradeSequence uint64,
+	upgradeVersion string,
+	previousVersion string,
+) (string, error) {
+	versionMetadata, err := types.MetadataFromVersion(upgradeVersion)
+	if err != nil {
+		// fee version is unable to be parsed from upgrade version, disable fee
+		im.keeper.DeleteFeeEnabled(ctx, portID, channelID)
+		// since it is valid for fee version to not be specified, the upgrade version may be for a middleware
+		// or application further down in the stack. Thus, passthrough to next middleware or application in callstack.
+		return im.app.OnChanUpgradeInit(ctx, portID, channelID, order, connectionHops, upgradeSequence, upgradeVersion, previousVersion)
+	}
+
+	if versionMetadata.FeeVersion != types.Version {
+		return "", errorsmod.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, versionMetadata.FeeVersion)
+	}
+
+	appVersion, err := im.app.OnChanUpgradeInit(ctx, portID, channelID, order, connectionHops, upgradeSequence, versionMetadata.AppVersion, previousVersion)
+	if err != nil {
+		return "", err
+	}
+
+	versionMetadata.AppVersion = appVersion
+	versionBz, err := types.ModuleCdc.MarshalJSON(&versionMetadata)
+	if err != nil {
+		return "", err
+	}
+
+	im.keeper.SetFeeEnabled(ctx, portID, channelID)
+
+	return string(versionBz), nil
 }
 
 // OnChanUpgradeTry implement s the IBCModule interface

diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go
@@ -1004,6 +1004,118 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
 	}
 }
 
+func (suite *FeeTestSuite) TestOnChanUpgradeInit() {
+	var (
+		expFeeEnabled bool
+		path          *ibctesting.Path
+	)
+
+	testCases := []struct {
+		name     string
+		malleate func()
+		expError error
+	}{
+		{
+			"success",
+			func() {},
+			nil,
+		},
+		{
+			"success disable fees",
+			func() {
+				// create a new path using a fee enabled channel and downgrade it to disable fees
+				expFeeEnabled = false
+				path = ibctesting.NewPath(suite.chainA, suite.chainB)
+
+				mockFeeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version}))
+				path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
+				path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
+				path.EndpointA.ChannelConfig.Version = mockFeeVersion
+				path.EndpointB.ChannelConfig.Version = mockFeeVersion
+
+				upgradeVersion := ibcmock.Version
+				path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
+				path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
+
+				suite.coordinator.Setup(path)
+			},
+			nil,
+		},
+		{
+			"invalid upgrade version",
+			func() {
+				expFeeEnabled = false
+				path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = "invalid-version"
+				path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = "invalid-version"
+
+				suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanUpgradeInit = func(_ sdk.Context, _, _ string, _ channeltypes.Order, _ []string, _ uint64, _, _ string) (string, error) {
+					// intentionally force the error here so we can assert that a passthrough occurs when fees should not be enabled for this channel
+					return "", ibcmock.MockApplicationCallbackError
+				}
+			},
+			ibcmock.MockApplicationCallbackError,
+		},
+		{
+			"invalid fee version",
+			func() {
+				expFeeEnabled = false
+				upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: "invalid-version", AppVersion: ibcmock.Version}))
+				path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
+				path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
+			},
+			types.ErrInvalidVersion,
+		},
+		{
+			"underlying app callback returns error",
+			func() {
+				expFeeEnabled = false
+				suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanUpgradeInit = func(_ sdk.Context, _, _ string, _ channeltypes.Order, _ []string, _ uint64, _, _ string) (string, error) {
+					return "", ibcmock.MockApplicationCallbackError
+				}
+			},
+			ibcmock.MockApplicationCallbackError,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		suite.Run(tc.name, func() {
+			suite.SetupTest()
+
+			path = ibctesting.NewPath(suite.chainA, suite.chainB)
+
+			// configure the initial path to create an unincentivized mock channel
+			path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
+			path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
+			path.EndpointA.ChannelConfig.Version = ibcmock.Version
+			path.EndpointB.ChannelConfig.Version = ibcmock.Version
+
+			suite.coordinator.Setup(path)
+
+			// configure the channel upgrade version to enabled ics29 fee middleware
+			expFeeEnabled = true
+			upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version}))
+			path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
+			path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
+
+			tc.malleate()
+
+			err := path.EndpointA.ChanUpgradeInit()
+
+			isFeeEnabled := suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
+			suite.Require().Equal(expFeeEnabled, isFeeEnabled)
+
+			expPass := tc.expError == nil
+			if expPass {
+				suite.Require().NoError(err)
+			} else {
+				suite.Require().Error(err)
+				suite.Require().ErrorIs(err, tc.expError)
+			}
+		})
+	}
+}
+
 func (suite *FeeTestSuite) TestGetAppVersion() {
 	var (
 		portID        string

diff --git a/modules/apps/29-fee/types/metadata.go b/modules/apps/29-fee/types/metadata.go
@@ -0,0 +1,15 @@
+package types
+
+import errorsmod "cosmossdk.io/errors"
+
+// MetadataFromVersion attempts to parse the given string into a fee version Metadata,
+// an error is returned if it fails to do so.
+func MetadataFromVersion(version string) (Metadata, error) {
+	var metadata Metadata
+	err := ModuleCdc.UnmarshalJSON([]byte(version), &metadata)
+	if err != nil {
+		return Metadata{}, errorsmod.Wrapf(ErrInvalidVersion, "failed to unmarshal metadata from version: %s", version)
+	}
+
+	return metadata, nil
+}
diff --git a/modules/apps/29-fee/types/metadata_test.go b/modules/apps/29-fee/types/metadata_test.go
@@ -0,0 +1,29 @@
+package types_test
+
+import (
+	"testing"
+
+	"github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types"
+	ibcmock "github.com/cosmos/ibc-go/v7/testing/mock"
+	"github.com/stretchr/testify/require"
+)
+
+func TestMetadataFromVersion(t *testing.T) {
+	testMetadata := types.Metadata{
+		AppVersion: ibcmock.Version,
+		FeeVersion: types.Version,
+	}
+
+	versionBz, err := types.ModuleCdc.MarshalJSON(&testMetadata)
+	require.NoError(t, err)
+
+	metadata, err := types.MetadataFromVersion(string(versionBz))
+	require.NoError(t, err)
+	require.Equal(t, ibcmock.Version, metadata.AppVersion)
+	require.Equal(t, types.Version, metadata.FeeVersion)
+
+	metadata, err = types.MetadataFromVersion("")
+	require.Error(t, err)
+	require.ErrorIs(t, err, types.ErrInvalidVersion)
+	require.Empty(t, metadata)
+}
diff --git a/testing/chain.go b/testing/chain.go
@@ -318,14 +318,13 @@ func (chain *TestChain) SendMsgs(msgs ...sdk.Msg) (*sdk.Result, error) {
 	chain.Coordinator.UpdateTimeForChain(chain)
 
 	_, r, err := simapp.SignAndDeliver(
-		chain.TB,
 		chain.TxConfig,
 		chain.App.GetBaseApp(),
 		msgs,
 		chain.ChainID,
 		[]uint64{chain.SenderAccount.GetAccountNumber()},
 		[]uint64{chain.SenderAccount.GetSequence()},
-		true, chain.SenderPrivKey,
+		chain.SenderPrivKey,
 	)
 	if err != nil {
 		return nil, err

diff --git a/testing/simapp/test_helpers.go b/testing/simapp/test_helpers.go
@@ -3,7 +3,6 @@ package simapp
 import (
 	"encoding/json"
 	"math/rand"
-	"testing"
 	"time"
 
 	dbm "github.com/cometbft/cometbft-db"
@@ -22,7 +21,6 @@ import (
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
-	"github.com/stretchr/testify/require"
 
 	"github.com/cosmos/ibc-go/v7/testing/mock"
 )
@@ -209,8 +207,8 @@ func SetupWithGenesisAccounts(genAccs []authtypes.GenesisAccount, balances ...ba
 //
 // CONTRACT: BeginBlock must be called before this function.
 func SignAndDeliver(
-	tb testing.TB, txCfg client.TxConfig, app *bam.BaseApp, msgs []sdk.Msg,
-	chainID string, accNums, accSeqs []uint64, expPass bool, priv ...cryptotypes.PrivKey,
+	txCfg client.TxConfig, app *bam.BaseApp, msgs []sdk.Msg,
+	chainID string, accNums, accSeqs []uint64, priv ...cryptotypes.PrivKey,
 ) (sdk.GasInfo, *sdk.Result, error) {
 	tx, err := simtestutil.GenSignedMockTx(
 		rand.New(rand.NewSource(time.Now().UnixNano())),
@@ -223,18 +221,9 @@ func SignAndDeliver(
 		accSeqs,
 		priv...,
 	)
-	require.NoError(tb, err)
-
-	// Simulate a sending a transaction
-	gInfo, res, err := app.SimDeliver(txCfg.TxEncoder(), tx)
-
-	if expPass {
-		require.NoError(tb, err)
-		require.NotNil(tb, res)
-	} else {
-		require.Error(tb, err)
-		require.Nil(tb, res)
+	if err != nil {
+		return sdk.GasInfo{}, nil, err
 	}
 
-	return gInfo, res, err
+	return app.SimDeliver(txCfg.TxEncoder(), tx)
 }