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.

Sign up for GitHub

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

Jump to bottom

test: adding tests for OnRecvPacket #412

Merged
merged 2 commits into from
Sep 23, 2021
Merged

test: adding tests for OnRecvPacket #412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22a18e3
test: adding tests for OnRecvPacket
seantking Sep 15, 2021
a44c402
test: adding further test cases for onRecvPacket
seantking Sep 16, 2021
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
13 changes: 1 addition & 12 deletions modules/apps/27-interchain-accounts/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,7 @@ func (k Keeper) createOutgoingPacket(
return []byte{}, types.ErrInvalidOutgoingData
}

var msgs []sdk.Msg
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are doing this check inside SerializeCosmosTx(). I can't think of any added value to having it twice.


switch data := data.(type) {
case []sdk.Msg:
msgs = data
case sdk.Msg:
msgs = []sdk.Msg{data}
default:
return []byte{}, types.ErrInvalidOutgoingData
}

txBytes, err := k.SerializeCosmosTx(k.cdc, msgs)
txBytes, err := k.SerializeCosmosTx(k.cdc, data)
if err != nil {
return []byte{}, sdkerrors.Wrap(err, "invalid packet data or codec")
}
Expand Down
138 changes: 136 additions & 2 deletions modules/apps/27-interchain-accounts/keeper/relay_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
package keeper_test

import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"

"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"

seantking marked this conversation as resolved.
Show resolved Hide resolved
ibctesting "github.com/cosmos/ibc-go/testing"
)

Expand Down Expand Up @@ -74,13 +80,15 @@ func (suite *KeeperTestSuite) TestTrySendTx() {
suite.Run(tc.name, func() {
suite.SetupTest() // reset
path = NewICAPath(suite.chainA, suite.chainB)
owner := TestOwnerAddress
suite.coordinator.SetupConnections(path)

err := suite.SetupICAPath(path, owner)
err := suite.SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

portID = path.EndpointA.ChannelConfig.PortID

tc.malleate()

_, err = suite.chainA.GetSimApp().ICAKeeper.TrySendTx(suite.chainA.GetContext(), portID, msg)

if tc.expPass {
Expand All @@ -91,3 +99,129 @@ func (suite *KeeperTestSuite) TestTrySendTx() {
})
}
}

func (suite *KeeperTestSuite) TestOnRecvPacket() {
var (
path *ibctesting.Path
msg sdk.Msg
txBytes []byte
packetData []byte
sourcePort string
)

testCases := []struct {
msg string
malleate func()
expPass bool
}{
{
"Interchain account successfully executes banktypes.MsgSend", func() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@colin-axner I think we should test for multiple message types here in the future. At least the ones that we know are required for our initial use cases i.e. staking.

I'll do this in a follow-up PR (I need to look into staking a bit).

wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me, maybe we can try send an ibc-transfer?

// build MsgSend
amount, _ := sdk.ParseCoinsNormalized("100stake")
interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID)
msg = &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount}
Copy link
Member

Choose a reason for hiding this comment

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

Just a small nit, I think it would increase readability to instantiate on multiple lines like:

msg = &banktypes.MsgSend{
    FromAddress: interchainAccountAddr, 
    ToAddress: suite.chainB.SenderAccount.GetAddress().String(), 
    Amount: amount,
}

Same below for data in other testcases:

data := types.IBCAccountPacketData{
    Type: types.EXECUTE_TX,
    Data: txBytes,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion. I prefer it as well. I refactored some of the other tests also.

// build packet data
txBytes, err := suite.chainA.GetSimApp().ICAKeeper.SerializeCosmosTx(suite.chainA.Codec, msg)
suite.Require().NoError(err)

data := types.IBCAccountPacketData{Type: types.EXECUTE_TX,
Data: txBytes}
packetData = data.GetBytes()
}, true,
},
{
"Cannot deserialize txBytes", func() {
Copy link
Member

Choose a reason for hiding this comment

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

another minor nit: I think the usual standard is to keep testcase names in lowercase. Or at least that's what I'm generally used to seeing

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a fan of lowercase, but I don't think there's a good reason most tests are lowercase (aside from me probably having written them). Open to changing, but otherwise agree best to stick to what's already used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Updated 👍

txBytes = []byte("invalid tx bytes")
data := types.IBCAccountPacketData{Type: types.EXECUTE_TX,
Data: txBytes}
packetData = data.GetBytes()
}, false,
},
{
"Cannot deserialize txBytes: invalid IBCTxRaw", func() {
txBody := []byte("invalid tx body")
txRaw := &types.IBCTxRaw{
BodyBytes: txBody,
}

txBytes = suite.chainB.Codec.MustMarshal(txRaw)
data := types.IBCAccountPacketData{Type: types.EXECUTE_TX,
Data: txBytes}
packetData = data.GetBytes()
}, false,
},
{
"Invalid packet type", func() {
txBytes = []byte{}
// Type here is an ENUM
// Valid type is types.EXECUTE_TX
Copy link
Member

Choose a reason for hiding this comment

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

Is there a zero value enum for IBCAccountPacketData.Type that is UNKNOWN or UNSPECIFIED?
Protobuf normally recommends having the zero value enum as this for no-op or error scenarios and the enum types that you consume to invoke different code paths would come after.

For example:

enum Type {
    TYPE_UNSPECIFIED = 0;
    TYPE_EXECUTE_TX = 1;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this. Good catch & good to know. https://github.com/cosmos/ibc-go/pull/412/files#diff-5b066a6992f0f8e592492085bc4164eb73f17f8278ef7eafc064c5c197d80673R24

Copy link
Member

Choose a reason for hiding this comment

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

Excellent! 🤝

data := types.IBCAccountPacketData{Type: 100,
Data: txBytes}
packetData = data.GetBytes()
}, false,
},
{
"Cannot unmarshal interchain account packet data into types.IBCAccountPacketData", func() {
packetData = []byte{}
}, false,
},
{
"Unauthorised: Interchain account not found for given source portID", func() {
sourcePort = "invalid-port-id"
}, false,
},
{
"Unauthorised: Signer of message is not the interchain account associated with sourcePortID", func() {
// build MsgSend
amount, _ := sdk.ParseCoinsNormalized("100stake")
// Incorrect FromAddress
msg = &banktypes.MsgSend{FromAddress: suite.chainB.SenderAccount.GetAddress().String(), ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount}
// build packet data
txBytes, err := suite.chainA.GetSimApp().ICAKeeper.SerializeCosmosTx(suite.chainA.Codec, msg)
suite.Require().NoError(err)
data := types.IBCAccountPacketData{Type: types.EXECUTE_TX,
Data: txBytes}
packetData = data.GetBytes()
}, false,
},
}

for _, tc := range testCases {
tc := tc

suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
suite.Run(tc.msg, func() {

This is what I'll usually do, but happy to adjust if it is useful to prefix with Case

suite.SetupTest() // reset

path = NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)
err := suite.SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

// send 100stake to interchain account wallet
amount, _ := sdk.ParseCoinsNormalized("100stake")
interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID)
bankMsg := &banktypes.MsgSend{FromAddress: suite.chainB.SenderAccount.GetAddress().String(), ToAddress: interchainAccountAddr, Amount: amount}

_, err = suite.chainB.SendMsgs(bankMsg)
suite.Require().NoError(err)

// valid source port
sourcePort = path.EndpointA.ChannelConfig.PortID

// malleate packetData for test cases
tc.malleate()

seq := uint64(1)
packet := channeltypes.NewPacket(packetData, seq, sourcePort, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.NewHeight(0, 100), 0)

// Pass it in here
err = suite.chainB.GetSimApp().ICAKeeper.OnRecvPacket(suite.chainB.GetContext(), packet)

if tc.expPass {
suite.Require().NoError(err)
} else {
suite.Require().Error(err)
}
})
}
}