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

Implement MsgSendPacket RPC handler and tests #7413

Merged
Changes from 5 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
5 changes: 5 additions & 0 deletions modules/core/04-channel/v2/keeper/keeper.go
Original file line number Diff line number Diff line change
@@ -20,6 +20,7 @@ import (
commitmentv2types "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2"
host "github.com/cosmos/ibc-go/v9/modules/core/24-host"
hostv2 "github.com/cosmos/ibc-go/v9/modules/core/24-host/v2"
"github.com/cosmos/ibc-go/v9/modules/core/api"
"github.com/cosmos/ibc-go/v9/modules/core/exported"
)

@@ -31,6 +32,10 @@ type Keeper struct {
// channelKeeperV1 is used for channel aliasing only.
channelKeeperV1 *channelkeeperv1.Keeper
connectionKeeper *connectionkeeper.Keeper

// Router is used to route messages to the appropriate module callbacks
// NOTE: it must be explicitly set before usage.
Router *api.Router
}

// NewKeeper creates a new channel v2 keeper
30 changes: 13 additions & 17 deletions modules/core/04-channel/v2/keeper/msg_server.go
Original file line number Diff line number Diff line change
@@ -16,9 +16,9 @@ var _ channeltypesv2.MsgServer = &Keeper{}
// SendPacket implements the PacketMsgServer SendPacket method.
func (k *Keeper) SendPacket(ctx context.Context, msg *channeltypesv2.MsgSendPacket) (*channeltypesv2.MsgSendPacketResponse, error) {
sdkCtx := sdk.UnwrapSDKContext(ctx)
sequence, err := k.sendPacket(ctx, msg.SourceChannel, msg.TimeoutTimestamp, msg.PacketData)
sequence, destChannel, err := k.sendPacket(ctx, msg.SourceChannel, msg.TimeoutTimestamp, msg.PacketData)
if err != nil {
sdkCtx.Logger().Error("send packet failed", "source-id", msg.SourceChannel, "error", errorsmod.Wrap(err, "send packet failed"))
sdkCtx.Logger().Error("send packet failed", "source-channel", msg.SourceChannel, "error", errorsmod.Wrap(err, "send packet failed"))
return nil, errorsmod.Wrapf(err, "send packet failed for source id: %s", msg.SourceChannel)
}

@@ -28,17 +28,13 @@ func (k *Keeper) SendPacket(ctx context.Context, msg *channeltypesv2.MsgSendPack
return nil, errorsmod.Wrap(err, "invalid address for msg Signer")
}

_ = signer

// TODO: implement once app router is wired up.
// https://github.com/cosmos/ibc-go/issues/7384
// for _, pd := range msg.PacketData {
// cbs := k.PortKeeper.AppRouter.Route(pd.SourcePort)
// err := cbs.OnSendPacket(ctx, msg.SourceChannel, sequence, msg.TimeoutTimestamp, pd, signer)
// if err != nil {
// return nil, err
// }
// }
for _, pd := range msg.PacketData {
Copy link
Member

Choose a reason for hiding this comment

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

nit: i prefer calling this payload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

atm we have PacketData which contains a payload, but I'm fine with calling the entire thing payload and not having a nested field at all. Might do this in a follow up since there are some other PRs working with this same type, will make an issue to track this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cbs := k.Router.Route(pd.SourcePort)
err := cbs.OnSendPacket(ctx, msg.SourceChannel, destChannel, sequence, pd, signer)
if err != nil {
return nil, err
}
}

return &channeltypesv2.MsgSendPacketResponse{Sequence: sequence}, nil
}
@@ -59,10 +55,10 @@ func (k *Keeper) Acknowledgement(ctx context.Context, msg *channeltypesv2.MsgAck
writeFn()
case channeltypesv1.ErrNoOpMsg:
// no-ops do not need event emission as they will be ignored
sdkCtx.Logger().Debug("no-op on redundant relay", "source-id", msg.Packet.SourceChannel)
sdkCtx.Logger().Debug("no-op on redundant relay", "source-channel", msg.Packet.SourceChannel)
return &channeltypesv2.MsgAcknowledgementResponse{Result: channeltypesv1.NOOP}, nil
default:
sdkCtx.Logger().Error("acknowledgement failed", "source-id", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "acknowledge packet verification failed"))
sdkCtx.Logger().Error("acknowledgement failed", "source-channel", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "acknowledge packet verification failed"))
return nil, errorsmod.Wrap(err, "acknowledge packet verification failed")
}

@@ -86,7 +82,7 @@ func (k *Keeper) RecvPacket(ctx context.Context, msg *channeltypesv2.MsgRecvPack
sdkCtx := sdk.UnwrapSDKContext(ctx)
err := k.recvPacket(ctx, msg.Packet, msg.ProofCommitment, msg.ProofHeight)
if err != nil {
sdkCtx.Logger().Error("receive packet failed", "source-id", msg.Packet.SourceChannel, "dest-id", msg.Packet.DestinationChannel, "error", errorsmod.Wrap(err, "send packet failed"))
sdkCtx.Logger().Error("receive packet failed", "source-channel", msg.Packet.SourceChannel, "dest-channel", msg.Packet.DestinationChannel, "error", errorsmod.Wrap(err, "send packet failed"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

few drive by fixes to update the log lines

return nil, errorsmod.Wrapf(err, "receive packet failed for source id: %s and destination id: %s", msg.Packet.SourceChannel, msg.Packet.DestinationChannel)
}

@@ -115,7 +111,7 @@ func (k *Keeper) RecvPacket(ctx context.Context, msg *channeltypesv2.MsgRecvPack
func (k *Keeper) Timeout(ctx context.Context, timeout *channeltypesv2.MsgTimeout) (*channeltypesv2.MsgTimeoutResponse, error) {
sdkCtx := sdk.UnwrapSDKContext(ctx)
if err := k.timeoutPacket(ctx, timeout.Packet, timeout.ProofUnreceived, timeout.ProofHeight); err != nil {
sdkCtx.Logger().Error("Timeout packet failed", "source-id", timeout.Packet.SourceChannel, "destination-id", timeout.Packet.DestinationChannel, "error", errorsmod.Wrap(err, "timeout packet failed"))
sdkCtx.Logger().Error("Timeout packet failed", "source-channel", timeout.Packet.SourceChannel, "destination-channel", timeout.Packet.DestinationChannel, "error", errorsmod.Wrap(err, "timeout packet failed"))
return nil, errorsmod.Wrapf(err, "send packet failed for source id: %s and destination id: %s", timeout.Packet.SourceChannel, timeout.Packet.DestinationChannel)
}

113 changes: 113 additions & 0 deletions modules/core/04-channel/v2/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
package keeper_test

import (
"context"
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"

clienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types"
channeltypesv1 "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
channeltypesv2 "github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types"
ibctesting "github.com/cosmos/ibc-go/v9/testing"
"github.com/cosmos/ibc-go/v9/testing/mock"
mockv2 "github.com/cosmos/ibc-go/v9/testing/mock/v2"
)

func (suite *KeeperTestSuite) TestMsgSendPacket() {
var (
path *ibctesting.Path
msg *channeltypesv2.MsgSendPacket
expectedPacket channeltypesv2.Packet
)

testCases := []struct {
name string
malleate func()
expError error
}{
{
name: "success",
malleate: func() {},
expError: nil,
},
{
name: "failure: timeout elapsed",
malleate: func() {
// ensure a message timeout.
msg.TimeoutTimestamp = uint64(1)
},
expError: channeltypesv1.ErrTimeoutElapsed,
},
{
name: "failure: inactive client",
malleate: func() {
path.EndpointA.FreezeClient()
},
expError: clienttypes.ErrClientNotActive,
},
{
name: "failure: application callback error",
malleate: func() {
path.EndpointA.Chain.GetSimApp().MockModuleV2A.IBCApp.OnSendPacket = func(ctx context.Context, sourceID string, destinationID string, sequence uint64, data channeltypesv2.PacketData, signer sdk.AccAddress) error {
return mock.MockApplicationCallbackError
}
},
expError: mock.MockApplicationCallbackError,
},
{
name: "failure: counterparty not found",
malleate: func() {
msg.SourceChannel = "foo"
},
expError: channeltypesv1.ErrChannelNotFound,
},
{
name: "failure: route to non existing app",
malleate: func() {
msg.PacketData[0].SourcePort = "foo"
},
expError: fmt.Errorf("no route for foo"),
},
}

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

suite.Run(tc.name, func() {
suite.SetupTest() // reset

path = ibctesting.NewPath(suite.chainA, suite.chainB)
path.SetupV2()

timeoutTimestamp := suite.chainA.GetTimeoutTimestamp()
msg = channeltypesv2.NewMsgSendPacket(path.EndpointA.ChannelID, timeoutTimestamp, suite.chainA.SenderAccount.GetAddress().String(), mockv2.NewMockPacketData(mockv2.ModuleNameA, mockv2.ModuleNameB))

expectedPacket = channeltypesv2.NewPacket(1, path.EndpointA.ChannelID, path.EndpointB.ChannelID, timeoutTimestamp, mockv2.NewMockPacketData(mockv2.ModuleNameA, mockv2.ModuleNameB))

tc.malleate()

res, err := path.EndpointA.Chain.SendMsgs(msg)

expPass := tc.expError == nil
if expPass {
suite.Require().NoError(err)
suite.Require().NotNil(res)

ck := path.EndpointA.Chain.GetSimApp().IBCKeeper.ChannelKeeperV2

packetCommitment := ck.GetPacketCommitment(path.EndpointA.Chain.GetContext(), path.EndpointA.ChannelID, 1)
suite.Require().NotNil(packetCommitment)
suite.Require().Equal(channeltypesv2.CommitPacket(expectedPacket), packetCommitment, "packet commitment is not stored correctly")

nextSequenceSend, ok := ck.GetNextSequenceSend(path.EndpointA.Chain.GetContext(), path.EndpointA.ChannelID)
suite.Require().True(ok)
suite.Require().Equal(uint64(2), nextSequenceSend, "next sequence send was not incremented correctly")

} else {
suite.Require().Error(err)
ibctesting.RequireErrorIsOrContains(suite.T(), err, tc.expError)
}
})
}
}
21 changes: 10 additions & 11 deletions modules/core/04-channel/v2/keeper/packet.go
Original file line number Diff line number Diff line change
@@ -24,18 +24,18 @@ func (k *Keeper) sendPacket(
sourceID string,
timeoutTimestamp uint64,
data []channeltypesv2.PacketData,
) (uint64, error) {
) (uint64, string, error) {
// Lookup channel associated with our source channel to retrieve the destination channel
channel, ok := k.GetChannel(ctx, sourceID)
if !ok {
// TODO: figure out how aliasing will work when more than one packet data is sent.
channel, ok = k.convertV1Channel(ctx, data[0].SourcePort, sourceID)
if !ok {
return 0, errorsmod.Wrap(types.ErrChannelNotFound, sourceID)
return 0, "", errorsmod.Wrap(types.ErrChannelNotFound, sourceID)
}
}

destID := channel.CounterpartyChannelId
destChannel := channel.CounterpartyChannelId
clientID := channel.ClientId

// retrieve the sequence send for this channel
@@ -46,31 +46,31 @@ func (k *Keeper) sendPacket(
}

// construct packet from given fields and channel state
packet := channeltypesv2.NewPacket(sequence, sourceID, destID, timeoutTimestamp, data...)
packet := channeltypesv2.NewPacket(sequence, sourceID, destChannel, timeoutTimestamp, data...)

if err := packet.ValidateBasic(); err != nil {
return 0, errorsmod.Wrapf(channeltypes.ErrInvalidPacket, "constructed packet failed basic validation: %v", err)
return 0, "", errorsmod.Wrapf(channeltypes.ErrInvalidPacket, "constructed packet failed basic validation: %v", err)
}

// check that the client of counterparty chain is still active
if status := k.ClientKeeper.GetClientStatus(ctx, clientID); status != exported.Active {
return 0, errorsmod.Wrapf(clienttypes.ErrClientNotActive, "client (%s) status is %s", clientID, status)
return 0, "", errorsmod.Wrapf(clienttypes.ErrClientNotActive, "client (%s) status is %s", clientID, status)
}

// retrieve latest height and timestamp of the client of counterparty chain
latestHeight := k.ClientKeeper.GetClientLatestHeight(ctx, clientID)
if latestHeight.IsZero() {
return 0, errorsmod.Wrapf(clienttypes.ErrInvalidHeight, "cannot send packet using client (%s) with zero height", clientID)
return 0, "", errorsmod.Wrapf(clienttypes.ErrInvalidHeight, "cannot send packet using client (%s) with zero height", clientID)
}

latestTimestamp, err := k.ClientKeeper.GetClientTimestampAtHeight(ctx, clientID, latestHeight)
if err != nil {
return 0, err
return 0, "", err
}
// check if packet is timed out on the receiving chain
timeout := channeltypes.NewTimeoutWithTimestamp(timeoutTimestamp)
if timeout.TimestampElapsed(latestTimestamp) {
return 0, errorsmod.Wrap(timeout.ErrTimeoutElapsed(latestHeight, latestTimestamp), "invalid packet timeout")
return 0, "", errorsmod.Wrap(timeout.ErrTimeoutElapsed(latestHeight, latestTimestamp), "invalid packet timeout")
}

commitment := channeltypesv2.CommitPacket(packet)
@@ -83,11 +83,10 @@ func (k *Keeper) sendPacket(

EmitSendPacketEvents(ctx, packet)

return sequence, nil
return sequence, destChannel, nil
}

// recvPacket implements the packet receiving logic required by a packet handler.

// The packet is checked for correctness including asserting that the packet was
// sent and received on clients which are counterparties for one another.
// If the packet has already been received a no-op error is returned.
11 changes: 11 additions & 0 deletions modules/core/04-channel/v2/types/msgs.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package types

// NewMsgSendPacket creates a new MsgSendPacket instance.
func NewMsgSendPacket(sourceChannel string, timeoutTimestamp uint64, signer string, packetData ...PacketData) *MsgSendPacket {
return &MsgSendPacket{
SourceChannel: sourceChannel,
TimeoutTimestamp: timeoutTimestamp,
PacketData: packetData,
Signer: signer,
}
}
2 changes: 1 addition & 1 deletion modules/core/keeper/keeper.go
Original file line number Diff line number Diff line change
@@ -86,7 +86,7 @@ func (k *Keeper) SetRouter(rtr *porttypes.Router) {

// SetRouterV2 sets the v2 router for the IBC Keeper.
func (k *Keeper) SetRouterV2(rtr *api.Router) {
k.PortKeeper.RouterV2 = rtr
k.ChannelKeeperV2.Router = rtr
Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes a bit more sense here I think, otherwise we introduce a dependence of the port router on the ChannelKeeperV2

}

// GetAuthority returns the ibc module's authority.
16 changes: 0 additions & 16 deletions simapp/app.go
Original file line number Diff line number Diff line change
@@ -119,13 +119,11 @@ import (
ibcclienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types"
ibcconnectiontypes "github.com/cosmos/ibc-go/v9/modules/core/03-connection/types"
porttypes "github.com/cosmos/ibc-go/v9/modules/core/05-port/types"
ibcapi "github.com/cosmos/ibc-go/v9/modules/core/api"
ibcexported "github.com/cosmos/ibc-go/v9/modules/core/exported"
ibckeeper "github.com/cosmos/ibc-go/v9/modules/core/keeper"
solomachine "github.com/cosmos/ibc-go/v9/modules/light-clients/06-solomachine"
ibctm "github.com/cosmos/ibc-go/v9/modules/light-clients/07-tendermint"
"github.com/cosmos/ibc-go/v9/testing/mock"
mockv2 "github.com/cosmos/ibc-go/v9/testing/mock/v2"
)

const appName = "SimApp"
@@ -202,9 +200,6 @@ type SimApp struct {
ICAAuthModule mock.IBCModule
FeeMockModule mock.IBCModule

MockModuleV2A mockv2.IBCModule
MockModuleV2B mockv2.IBCModule
Comment on lines -205 to -206
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrong simapp for these


// the module manager
ModuleManager *module.Manager
BasicModuleManager module.BasicManager
@@ -422,7 +417,6 @@ func NewSimApp(

// Create IBC Router
ibcRouter := porttypes.NewRouter()
ibcRouterV2 := ibcapi.NewRouter()

// Middleware Stacks

@@ -517,18 +511,8 @@ func NewSimApp(
feeWithMockModule := ibcfee.NewIBCMiddleware(feeMockModule, app.IBCFeeKeeper)
ibcRouter.AddRoute(MockFeePort, feeWithMockModule)

// create two separate mock v2 applications so that it is possible to test multi packet data.
mockV2A := mockv2.NewIBCModule()
ibcRouterV2.AddRoute(mockv2.ModuleNameA, mockV2A)
app.MockModuleV2A = mockV2A

mockV2B := mockv2.NewIBCModule()
ibcRouterV2.AddRoute(mockv2.ModuleNameB, mockV2B)
app.MockModuleV2B = mockV2B

// Set the IBC Routers
app.IBCKeeper.SetRouter(ibcRouter)
app.IBCKeeper.SetRouterV2(ibcRouterV2)

clientKeeper := app.IBCKeeper.ClientKeeper
storeProvider := clientKeeper.GetStoreProvider()
17 changes: 17 additions & 0 deletions testing/mock/v2/mock.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,22 @@
package mock

import (
"github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types"
"github.com/cosmos/ibc-go/v9/testing/mock"
)

const (
ModuleName = "mockv2"
)

func NewMockPacketData(sourcePort, destPort string) types.PacketData {
return types.PacketData{
SourcePort: sourcePort,
DestinationPort: destPort,
Payload: types.Payload{
Encoding: "json",
Value: mock.MockPacketData,
Version: mock.Version,
},
}
}
16 changes: 16 additions & 0 deletions testing/simapp/app.go
Original file line number Diff line number Diff line change
@@ -105,12 +105,14 @@ import (
ibcclienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types"
ibcconnectiontypes "github.com/cosmos/ibc-go/v9/modules/core/03-connection/types"
porttypes "github.com/cosmos/ibc-go/v9/modules/core/05-port/types"
ibcapi "github.com/cosmos/ibc-go/v9/modules/core/api"
ibcexported "github.com/cosmos/ibc-go/v9/modules/core/exported"
ibckeeper "github.com/cosmos/ibc-go/v9/modules/core/keeper"
packetserverkeeper "github.com/cosmos/ibc-go/v9/modules/core/packet-server/keeper"
solomachine "github.com/cosmos/ibc-go/v9/modules/light-clients/06-solomachine"
ibctm "github.com/cosmos/ibc-go/v9/modules/light-clients/07-tendermint"
ibcmock "github.com/cosmos/ibc-go/v9/testing/mock"
mockv2 "github.com/cosmos/ibc-go/v9/testing/mock/v2"
ibctestingtypes "github.com/cosmos/ibc-go/v9/testing/types"
)

@@ -181,6 +183,9 @@ type SimApp struct {
ICAAuthModule ibcmock.IBCModule
FeeMockModule ibcmock.IBCModule

MockModuleV2A mockv2.IBCModule
MockModuleV2B mockv2.IBCModule

// the module manager
ModuleManager *module.Manager
BasicModuleManager module.BasicManager
@@ -385,6 +390,7 @@ func NewSimApp(

// Create IBC Router
ibcRouter := porttypes.NewRouter()
ibcRouterV2 := ibcapi.NewRouter()

// Middleware Stacks

@@ -479,8 +485,18 @@ func NewSimApp(
feeWithMockModule := ibcfee.NewIBCMiddleware(feeMockModule, app.IBCFeeKeeper)
ibcRouter.AddRoute(MockFeePort, feeWithMockModule)

// create two separate mock v2 applications so that it is possible to test multi packet data.
mockV2A := mockv2.NewIBCModule()
ibcRouterV2.AddRoute(mockv2.ModuleNameA, mockV2A)
app.MockModuleV2A = mockV2A

mockV2B := mockv2.NewIBCModule()
ibcRouterV2.AddRoute(mockv2.ModuleNameB, mockV2B)
app.MockModuleV2B = mockV2B

// Seal the IBC Router
app.IBCKeeper.SetRouter(ibcRouter)
app.IBCKeeper.SetRouterV2(ibcRouterV2)

clientKeeper := app.IBCKeeper.ClientKeeper
storeProvider := app.IBCKeeper.ClientKeeper.GetStoreProvider()