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

Add missing channel upgrade tests #5502

Merged
merged 12 commits into from
Jan 4, 2024
7 changes: 7 additions & 0 deletions modules/apps/transfer/ibc_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() {
{
"success", func() {}, true,
},
{
// connection hops is not used in the transfer application callback,
// it is already validated in the core OnChanUpgradeInit.
"success: invalid connection hops", func() {
path.EndpointA.ConnectionID = "invalid-connection-id"
}, true,
},
{
"empty version string", func() {
channel.Version = ""
Expand Down
3 changes: 3 additions & 0 deletions modules/core/04-channel/types/msgs.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ var (
_ sdk.HasValidateBasic = (*MsgChannelUpgradeTry)(nil)
_ sdk.HasValidateBasic = (*MsgChannelUpgradeAck)(nil)
_ sdk.HasValidateBasic = (*MsgChannelUpgradeConfirm)(nil)
_ sdk.HasValidateBasic = (*MsgChannelUpgradeTimeout)(nil)
_ sdk.HasValidateBasic = (*MsgChannelUpgradeCancel)(nil)
_ sdk.HasValidateBasic = (*MsgPruneAcknowledgements)(nil)
)

// NewMsgChannelOpenInit creates a new MsgChannelOpenInit. It sets the counterparty channel
Expand Down
58 changes: 58 additions & 0 deletions modules/core/04-channel/types/msgs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"testing"


dbm "github.com/cosmos/cosmos-db"
testifysuite "github.com/stretchr/testify/suite"

Expand All @@ -15,6 +16,8 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"
moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"

ibc "github.com/cosmos/ibc-go/v8/modules/core"
clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
Expand Down Expand Up @@ -1255,6 +1258,61 @@ func (suite *TypesTestSuite) TestMsgPruneAcknowledgementsValidateBasic() {
}
}

func (suite *TypesTestSuite) TestMsgUpdateParamsValidateBasic() {
var msg *types.MsgUpdateParams

testCases := []struct {
name string
malleate func()
expErr error
}{
{
"success",
func() {},
nil,
},
{
"invalid authority",
func() {
msg.Authority = "invalid-address"
},
ibcerrors.ErrInvalidAddress,
},
{
"invalid params: non zero height",
func() {
newHeight := clienttypes.NewHeight(1, 1000)
msg = types.NewMsgUpdateChannelParams(authtypes.NewModuleAddress(govtypes.ModuleName).String(), types.NewParams(types.NewTimeout(newHeight, uint64(100000))))
},
types.ErrInvalidTimeout,
},
{
"invalid params: zero timestamp",
func() {
msg = types.NewMsgUpdateChannelParams(authtypes.NewModuleAddress(govtypes.ModuleName).String(), types.NewParams(types.NewTimeout(clienttypes.ZeroHeight(), uint64(0))))
},
types.ErrInvalidTimeout,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
msg = types.NewMsgUpdateChannelParams(authtypes.NewModuleAddress(govtypes.ModuleName).String(), types.NewParams(types.NewTimeout(clienttypes.ZeroHeight(), uint64(100000))))

tc.malleate()
err := msg.ValidateBasic()

expPass := tc.expErr == nil
if expPass {
suite.Require().NoError(err)
} else {
suite.Require().ErrorIs(err, tc.expErr)
}
})
}
}

func (suite *TypesTestSuite) TestMsgPruneAcknowledgementsGetSigners() {
expSigner, err := sdk.AccAddressFromBech32(addr)
suite.Require().NoError(err)
Expand Down
8 changes: 5 additions & 3 deletions modules/core/04-channel/types/params.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package types

import (
"fmt"
"time"


errorsmod "cosmossdk.io/errors"

clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
)

Expand All @@ -27,10 +29,10 @@ func DefaultParams() Params {
// Validate the params.
func (p Params) Validate() error {
if !p.UpgradeTimeout.Height.IsZero() {
return fmt.Errorf("upgrade timeout height must be zero. got : %v", p.UpgradeTimeout.Height)
return errorsmod.Wrapf(ErrInvalidTimeout, "upgrade timeout height must be zero. got : %v", p.UpgradeTimeout.Height)
Copy link
Contributor

@DimitrisJim DimitrisJim Jan 2, 2024

Choose a reason for hiding this comment

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

this seems to be 'invalid packet timeout' yea? Do we want separate error? I'm actually not finding other references for that error tbh 🤔

edit: we could just tweak err message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, it looks like we already have a ErrInvalidUpgradeTimeout, we can use that instead!

}
if p.UpgradeTimeout.Timestamp == 0 {
return fmt.Errorf("upgrade timeout timestamp invalid: %v", p.UpgradeTimeout.Timestamp)
return errorsmod.Wrapf(ErrInvalidTimeout, "upgrade timeout timestamp invalid: %v", p.UpgradeTimeout.Timestamp)
}
return nil
}
52 changes: 52 additions & 0 deletions modules/core/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2017,6 +2017,58 @@ func (suite *KeeperTestSuite) TestUpdateConnectionParams() {
}
}

// TestUpdateChannelParams tests the UpdateChannelParams rpc handler
func (suite *KeeperTestSuite) TestUpdateChannelParams() {
signer := suite.chainA.App.GetIBCKeeper().GetAuthority()
testCases := []struct {
name string
msg *channeltypes.MsgUpdateParams
expError error
}{
{
"success: valid signer and default params",
channeltypes.NewMsgUpdateChannelParams(signer, channeltypes.DefaultParams()),
nil,
},
{
"failure: malformed signer",
channeltypes.NewMsgUpdateChannelParams(ibctesting.InvalidID, channeltypes.DefaultParams()),
ibcerrors.ErrUnauthorized,
},
{
"failure: whitespace signer",
channeltypes.NewMsgUpdateChannelParams(" ", channeltypes.DefaultParams()),
ibcerrors.ErrUnauthorized,
},
{
"failure: empty signer",
channeltypes.NewMsgUpdateChannelParams("", channeltypes.DefaultParams()),
ibcerrors.ErrUnauthorized,
},
{
"failure: unauthorized signer",
channeltypes.NewMsgUpdateChannelParams(ibctesting.TestAccAddress, channeltypes.DefaultParams()),
ibcerrors.ErrUnauthorized,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()
_, err := keeper.Keeper.UpdateChannelParams(*suite.chainA.App.GetIBCKeeper(), suite.chainA.GetContext(), tc.msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

want to also add a check for nil/non-nil assert on the resp?

expPass := tc.expError == nil
if expPass {
suite.Require().NoError(err)
p := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetParams(suite.chainA.GetContext())
suite.Require().Equal(tc.msg.Params, p)
} else {
suite.Require().ErrorIs(tc.expError, err)
}
})
}
}

func (suite *KeeperTestSuite) TestPruneAcknowledgements() {
var msg *channeltypes.MsgPruneAcknowledgements

Expand Down