Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding OnChanUpgradeInit handler implementation to 29-fee #4019

Merged
merged 14 commits into from
Jul 17, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 37 additions & 2 deletions modules/apps/29-fee/ibc_middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved

return string(versionBz), nil
}

// OnChanUpgradeTry implement s the IBCModule interface
Expand Down
113 changes: 113 additions & 0 deletions modules/apps/29-fee/ibc_middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1004,6 +1004,119 @@ 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,
},
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
{
"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 results in isFeeEnabled == false
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
return "", ibcmock.MockApplicationCallbackError
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}
},
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
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
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)

expPass := tc.expError == nil
if expPass {
suite.Require().Equal(expFeeEnabled, isFeeEnabled)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
suite.Require().NoError(err)
} else {
suite.Require().Equal(expFeeEnabled, isFeeEnabled)
suite.Require().Error(err)
suite.Require().ErrorIs(err, tc.expError)
}
})
}
}

func (suite *FeeTestSuite) TestGetAppVersion() {
var (
portID string
Expand Down
13 changes: 13 additions & 0 deletions modules/apps/29-fee/types/metadata.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package types

// 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) {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
var metadata Metadata
err := ModuleCdc.UnmarshalJSON([]byte(version), &metadata)
if err != nil {
return Metadata{}, ErrInvalidVersion
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we indicate that the version could not be unmarshaled into the metadata?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally was thinking that this would be wrapped by the caller like for example: errorsmod.Wrapf(err, "failed to unmarshal ICS29 counterparty version metadata: %s)

But I like this suggestion! Changed it to return this here: errorsmod.Wrapf(ErrInvalidVersion, "failed to unmarshal metadata from version: %s", version). From caller we can just Wrap(err, "invalid counterparty version")

}

return metadata, nil
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
29 changes: 29 additions & 0 deletions modules/apps/29-fee/types/metadata_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
3 changes: 1 addition & 2 deletions testing/chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 5 additions & 16 deletions testing/simapp/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package simapp
import (
"encoding/json"
"math/rand"
"testing"
"time"

dbm "github.com/cometbft/cometbft-db"
Expand All @@ -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"
)
Expand Down Expand Up @@ -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())),
Expand All @@ -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)
}
Loading