From 366ab9d481512748b5349db6e8bdaeafe0ee262f Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 8 Aug 2024 17:09:10 +0300 Subject: [PATCH] feat(core): Wire packet handler to core message server (#7091) * chore: return app version in handlers * add expected keeper interface for packet handler functions. * add switch in msg_server to dispatch based on protocol version. * guard TimeoutExecuted with version check for time being. * rename interface. * inline timeoutExecuted * slipped WriteAck. * use msg-server entrypoints for packet flow in testing. * use endpoint.SendPacket in recv test. --- modules/core/04-channel/keeper/export_test.go | 6 ++ modules/core/04-channel/keeper/timeout.go | 15 ++- .../core/04-channel/keeper/timeout_test.go | 7 +- modules/core/keeper/expected_keeper.go | 43 ++++++++ modules/core/keeper/keeper.go | 24 +++-- modules/core/keeper/msg_server.go | 98 +++++++++++++------ modules/core/packet-server/keeper/keeper.go | 53 +++++----- .../core/packet-server/keeper/keeper_test.go | 48 +++------ testing/endpoint.go | 27 +++++ 9 files changed, 216 insertions(+), 105 deletions(-) create mode 100644 modules/core/keeper/expected_keeper.go diff --git a/modules/core/04-channel/keeper/export_test.go b/modules/core/04-channel/keeper/export_test.go index bbe084b15c5..24ac7f29061 100644 --- a/modules/core/04-channel/keeper/export_test.go +++ b/modules/core/04-channel/keeper/export_test.go @@ -7,6 +7,7 @@ package keeper import ( sdk "github.com/cosmos/cosmos-sdk/types" + capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types" "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types" ) @@ -34,3 +35,8 @@ func (k *Keeper) SetUpgradeErrorReceipt(ctx sdk.Context, portID, channelID strin func (k *Keeper) SetRecvStartSequence(ctx sdk.Context, portID, channelID string, sequence uint64) { k.setRecvStartSequence(ctx, portID, channelID, sequence) } + +// TimeoutExecuted is a wrapper around timeoutExecuted to allow the function to be directly called in tests. +func (k *Keeper) TimeoutExecuted(ctx sdk.Context, capability *capabilitytypes.Capability, packet types.Packet) error { + return k.timeoutExecuted(ctx, capability, packet) +} diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go index e834e5f8e82..90eefdb61a6 100644 --- a/modules/core/04-channel/keeper/timeout.go +++ b/modules/core/04-channel/keeper/timeout.go @@ -24,6 +24,7 @@ import ( // ante handler. func (k *Keeper) TimeoutPacket( ctx sdk.Context, + chanCap *capabilitytypes.Capability, packet types.Packet, proof []byte, proofHeight exported.Height, @@ -119,18 +120,21 @@ func (k *Keeper) TimeoutPacket( return "", err } - // NOTE: the remaining code is located in the TimeoutExecuted function + if err = k.timeoutExecuted(ctx, chanCap, packet); err != nil { + return "", err + } + return channel.Version, nil } -// TimeoutExecuted deletes the commitment send from this chain after it verifies timeout. +// timeoutExecuted deletes the commitment send from this chain after it verifies timeout. // If the timed-out packet came from an ORDERED channel then this channel will be closed. // If the channel is in the FLUSHING state and there is a counterparty upgrade, then the // upgrade will be aborted if the upgrade has timed out. Otherwise, if there are no more inflight packets, // then the channel will be set to the FLUSHCOMPLETE state. // // CONTRACT: this function must be called in the IBC handler -func (k *Keeper) TimeoutExecuted( +func (k *Keeper) timeoutExecuted( ctx sdk.Context, chanCap *capabilitytypes.Capability, packet types.Packet, @@ -298,6 +302,9 @@ func (k *Keeper) TimeoutOnClose( return "", err } - // NOTE: the remaining code is located in the TimeoutExecuted function + if err = k.timeoutExecuted(ctx, chanCap, packet); err != nil { + return "", err + } + return channel.Version, nil } diff --git a/modules/core/04-channel/keeper/timeout_test.go b/modules/core/04-channel/keeper/timeout_test.go index 26b55158907..1c9f77fa4f9 100644 --- a/modules/core/04-channel/keeper/timeout_test.go +++ b/modules/core/04-channel/keeper/timeout_test.go @@ -27,6 +27,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { var ( path *ibctesting.Path packet types.Packet + chanCap *capabilitytypes.Capability nextSeqRecv uint64 ordered bool expError *errorsmod.Error @@ -47,6 +48,8 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { // need to update chainA's client representing chainB to prove missing ack err = path.EndpointA.UpdateClient() suite.Require().NoError(err) + + chanCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, true}, {"success: UNORDERED", func() { ordered = false @@ -60,6 +63,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { // need to update chainA's client representing chainB to prove missing ack err = path.EndpointA.UpdateClient() suite.Require().NoError(err) + chanCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, true}, {"packet already timed out: ORDERED", func() { expError = types.ErrNoOpMsg @@ -144,6 +148,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { timeoutTimestamp := uint64(suite.chainB.GetContext().BlockTime().UnixNano()) sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, timeoutTimestamp, ibctesting.MockPacketData) + suite.Require().NoError(err) packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, timeoutTimestamp) err = path.EndpointA.UpdateClient() @@ -220,7 +225,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { } } - channelVersion, err := suite.chainA.App.GetIBCKeeper().ChannelKeeper.TimeoutPacket(suite.chainA.GetContext(), packet, proof, proofHeight, nextSeqRecv) + channelVersion, err := suite.chainA.App.GetIBCKeeper().ChannelKeeper.TimeoutPacket(suite.chainA.GetContext(), chanCap, packet, proof, proofHeight, nextSeqRecv) if tc.expPass { suite.Require().NoError(err) diff --git a/modules/core/keeper/expected_keeper.go b/modules/core/keeper/expected_keeper.go new file mode 100644 index 00000000000..31b2ca2cdb5 --- /dev/null +++ b/modules/core/keeper/expected_keeper.go @@ -0,0 +1,43 @@ +package keeper + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + + capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types" + channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types" + "github.com/cosmos/ibc-go/v9/modules/core/exported" +) + +type PacketHandler interface { + RecvPacket( + ctx sdk.Context, + chanCap *capabilitytypes.Capability, + packet channeltypes.Packet, + proof []byte, + proofHeight exported.Height) (string, error) + + WriteAcknowledgement( + ctx sdk.Context, + chanCap *capabilitytypes.Capability, + packet exported.PacketI, + acknowledgement exported.Acknowledgement, + ) error + + AcknowledgePacket( + ctx sdk.Context, + chanCap *capabilitytypes.Capability, + packet channeltypes.Packet, + acknowledgement []byte, + proof []byte, + proofHeight exported.Height, + ) (string, error) + + TimeoutPacket( + ctx sdk.Context, + chanCap *capabilitytypes.Capability, + packet channeltypes.Packet, + proof []byte, + proofHeight exported.Height, + nextSequenceRecv uint64, + ) (string, error) +} diff --git a/modules/core/keeper/keeper.go b/modules/core/keeper/keeper.go index fed78a94888..da389975107 100644 --- a/modules/core/keeper/keeper.go +++ b/modules/core/keeper/keeper.go @@ -16,15 +16,17 @@ import ( channelkeeper "github.com/cosmos/ibc-go/v9/modules/core/04-channel/keeper" portkeeper "github.com/cosmos/ibc-go/v9/modules/core/05-port/keeper" porttypes "github.com/cosmos/ibc-go/v9/modules/core/05-port/types" + packetserver "github.com/cosmos/ibc-go/v9/modules/core/packet-server/keeper" "github.com/cosmos/ibc-go/v9/modules/core/types" ) // Keeper defines each ICS keeper for IBC type Keeper struct { - ClientKeeper *clientkeeper.Keeper - ConnectionKeeper *connectionkeeper.Keeper - ChannelKeeper *channelkeeper.Keeper - PortKeeper *portkeeper.Keeper + ClientKeeper *clientkeeper.Keeper + ConnectionKeeper *connectionkeeper.Keeper + ChannelKeeper *channelkeeper.Keeper + PacketServerKeeper *packetserver.Keeper + PortKeeper *portkeeper.Keeper cdc codec.BinaryCodec @@ -54,14 +56,16 @@ func NewKeeper( connectionKeeper := connectionkeeper.NewKeeper(cdc, key, paramSpace, clientKeeper) portKeeper := portkeeper.NewKeeper(scopedKeeper) channelKeeper := channelkeeper.NewKeeper(cdc, key, clientKeeper, connectionKeeper, portKeeper, scopedKeeper) + packetKeeper := packetserver.NewKeeper(cdc, channelKeeper, clientKeeper) return &Keeper{ - cdc: cdc, - ClientKeeper: clientKeeper, - ConnectionKeeper: connectionKeeper, - ChannelKeeper: channelKeeper, - PortKeeper: portKeeper, - authority: authority, + cdc: cdc, + ClientKeeper: clientKeeper, + ConnectionKeeper: connectionKeeper, + ChannelKeeper: channelKeeper, + PacketServerKeeper: packetKeeper, + PortKeeper: portKeeper, + authority: authority, } } diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 3e112779371..d8b4a295326 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -3,11 +3,13 @@ package keeper import ( "context" "errors" + "fmt" errorsmod "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" + capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types" clienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types" connectiontypes "github.com/cosmos/ibc-go/v9/modules/core/03-connection/types" "github.com/cosmos/ibc-go/v9/modules/core/04-channel/keeper" @@ -460,6 +462,11 @@ func (k *Keeper) ChannelCloseConfirm(goCtx context.Context, msg *channeltypes.Ms // RecvPacket defines a rpc handler method for MsgRecvPacket. func (k *Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacket) (*channeltypes.MsgRecvPacketResponse, error) { + var ( + packetHandler PacketHandler + module string + capability *capabilitytypes.Capability + ) ctx := sdk.UnwrapSDKContext(goCtx) relayer, err := sdk.AccAddressFromBech32(msg.Signer) @@ -468,11 +475,22 @@ func (k *Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPack return nil, errorsmod.Wrap(err, "Invalid address for msg Signer") } - // Lookup module by channel capability - module, capability, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.DestinationPort, msg.Packet.DestinationChannel) - if err != nil { - ctx.Logger().Error("receive packet failed", "port-id", msg.Packet.SourcePort, "channel-id", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "could not retrieve module from port-id")) - return nil, errorsmod.Wrap(err, "could not retrieve module from port-id") + switch msg.Packet.ProtocolVersion { + case channeltypes.IBC_VERSION_UNSPECIFIED, channeltypes.IBC_VERSION_1: + packetHandler = k.ChannelKeeper + + // Lookup module by channel capability + module, capability, err = k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.DestinationPort, msg.Packet.DestinationChannel) + if err != nil { + ctx.Logger().Error("acknowledgement failed", "port-id", msg.Packet.SourcePort, "channel-id", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "could not retrieve module from port-id")) + return nil, errorsmod.Wrap(err, "could not retrieve module from port-id") + } + + case channeltypes.IBC_VERSION_2: + packetHandler = k.PacketServerKeeper + module = msg.Packet.DestinationPort + default: + panic(fmt.Errorf("unsupported protocol version %d", msg.Packet.ProtocolVersion)) } // Retrieve callbacks from router @@ -487,7 +505,7 @@ func (k *Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPack // If the packet was already received, perform a no-op // Use a cached context to prevent accidental state changes cacheCtx, writeFn := ctx.CacheContext() - channelVersion, err := k.ChannelKeeper.RecvPacket(cacheCtx, capability, msg.Packet, msg.ProofCommitment, msg.ProofHeight) + channelVersion, err := packetHandler.RecvPacket(cacheCtx, capability, msg.Packet, msg.ProofCommitment, msg.ProofHeight) switch err { case nil: @@ -518,7 +536,7 @@ func (k *Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPack // NOTE: IBC applications modules may call the WriteAcknowledgement asynchronously if the // acknowledgement is nil. if ack != nil { - if err := k.ChannelKeeper.WriteAcknowledgement(ctx, capability, msg.Packet, ack); err != nil { + if err := packetHandler.WriteAcknowledgement(ctx, capability, msg.Packet, ack); err != nil { return nil, err } } @@ -532,6 +550,11 @@ func (k *Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPack // Timeout defines a rpc handler method for MsgTimeout. func (k *Keeper) Timeout(goCtx context.Context, msg *channeltypes.MsgTimeout) (*channeltypes.MsgTimeoutResponse, error) { + var ( + packetHandler PacketHandler + module string + capability *capabilitytypes.Capability + ) ctx := sdk.UnwrapSDKContext(goCtx) relayer, err := sdk.AccAddressFromBech32(msg.Signer) @@ -540,11 +563,22 @@ func (k *Keeper) Timeout(goCtx context.Context, msg *channeltypes.MsgTimeout) (* return nil, errorsmod.Wrap(err, "Invalid address for msg Signer") } - // Lookup module by channel capability - module, capability, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.SourcePort, msg.Packet.SourceChannel) - if err != nil { - ctx.Logger().Error("timeout failed", "port-id", msg.Packet.SourcePort, "channel-id", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "could not retrieve module from port-id")) - return nil, errorsmod.Wrap(err, "could not retrieve module from port-id") + switch msg.Packet.ProtocolVersion { + case channeltypes.IBC_VERSION_UNSPECIFIED, channeltypes.IBC_VERSION_1: + packetHandler = k.ChannelKeeper + + // Lookup module by channel capability + module, capability, err = k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.SourcePort, msg.Packet.SourceChannel) + if err != nil { + ctx.Logger().Error("acknowledgement failed", "port-id", msg.Packet.SourcePort, "channel-id", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "could not retrieve module from port-id")) + return nil, errorsmod.Wrap(err, "could not retrieve module from port-id") + } + + case channeltypes.IBC_VERSION_2: + packetHandler = k.PacketServerKeeper + module = msg.Packet.SourcePort + default: + panic(fmt.Errorf("unsupported protocol version %d", msg.Packet.ProtocolVersion)) } // Retrieve callbacks from router @@ -559,7 +593,7 @@ func (k *Keeper) Timeout(goCtx context.Context, msg *channeltypes.MsgTimeout) (* // If the timeout was already received, perform a no-op // Use a cached context to prevent accidental state changes cacheCtx, writeFn := ctx.CacheContext() - channelVersion, err := k.ChannelKeeper.TimeoutPacket(cacheCtx, msg.Packet, msg.ProofUnreceived, msg.ProofHeight, msg.NextSequenceRecv) + channelVersion, err := packetHandler.TimeoutPacket(cacheCtx, capability, msg.Packet, msg.ProofUnreceived, msg.ProofHeight, msg.NextSequenceRecv) switch err { case nil: @@ -573,11 +607,6 @@ func (k *Keeper) Timeout(goCtx context.Context, msg *channeltypes.MsgTimeout) (* return nil, errorsmod.Wrap(err, "timeout packet verification failed") } - // Delete packet commitment - if err = k.ChannelKeeper.TimeoutExecuted(ctx, capability, msg.Packet); err != nil { - return nil, err - } - // Perform application logic callback err = cbs.OnTimeoutPacket(ctx, channelVersion, msg.Packet, relayer) if err != nil { @@ -635,11 +664,6 @@ func (k *Keeper) TimeoutOnClose(goCtx context.Context, msg *channeltypes.MsgTime return nil, errorsmod.Wrap(err, "timeout on close packet verification failed") } - // Delete packet commitment - if err = k.ChannelKeeper.TimeoutExecuted(ctx, capability, msg.Packet); err != nil { - return nil, err - } - // Perform application logic callback // // NOTE: MsgTimeout and MsgTimeoutOnClose use the same "OnTimeoutPacket" @@ -659,6 +683,11 @@ func (k *Keeper) TimeoutOnClose(goCtx context.Context, msg *channeltypes.MsgTime // Acknowledgement defines a rpc handler method for MsgAcknowledgement. func (k *Keeper) Acknowledgement(goCtx context.Context, msg *channeltypes.MsgAcknowledgement) (*channeltypes.MsgAcknowledgementResponse, error) { + var ( + packetHandler PacketHandler + module string + capability *capabilitytypes.Capability + ) ctx := sdk.UnwrapSDKContext(goCtx) relayer, err := sdk.AccAddressFromBech32(msg.Signer) @@ -667,11 +696,22 @@ func (k *Keeper) Acknowledgement(goCtx context.Context, msg *channeltypes.MsgAck return nil, errorsmod.Wrap(err, "Invalid address for msg Signer") } - // Lookup module by channel capability - module, capability, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.SourcePort, msg.Packet.SourceChannel) - if err != nil { - ctx.Logger().Error("acknowledgement failed", "port-id", msg.Packet.SourcePort, "channel-id", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "could not retrieve module from port-id")) - return nil, errorsmod.Wrap(err, "could not retrieve module from port-id") + switch msg.Packet.ProtocolVersion { + case channeltypes.IBC_VERSION_UNSPECIFIED, channeltypes.IBC_VERSION_1: + packetHandler = k.ChannelKeeper + + // Lookup module by channel capability + module, capability, err = k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.SourcePort, msg.Packet.SourceChannel) + if err != nil { + ctx.Logger().Error("acknowledgement failed", "port-id", msg.Packet.SourcePort, "channel-id", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "could not retrieve module from port-id")) + return nil, errorsmod.Wrap(err, "could not retrieve module from port-id") + } + + case channeltypes.IBC_VERSION_2: + packetHandler = k.PacketServerKeeper + module = msg.Packet.SourcePort + default: + panic(fmt.Errorf("unsupported protocol version %d", msg.Packet.ProtocolVersion)) } // Retrieve callbacks from router @@ -686,7 +726,7 @@ func (k *Keeper) Acknowledgement(goCtx context.Context, msg *channeltypes.MsgAck // If the acknowledgement was already received, perform a no-op // Use a cached context to prevent accidental state changes cacheCtx, writeFn := ctx.CacheContext() - channelVersion, err := k.ChannelKeeper.AcknowledgePacket(cacheCtx, capability, msg.Packet, msg.Acknowledgement, msg.ProofAcked, msg.ProofHeight) + channelVersion, err := packetHandler.AcknowledgePacket(cacheCtx, capability, msg.Packet, msg.Acknowledgement, msg.ProofAcked, msg.ProofHeight) switch err { case nil: diff --git a/modules/core/packet-server/keeper/keeper.go b/modules/core/packet-server/keeper/keeper.go index 975a1d0c018..7e8d37a0659 100644 --- a/modules/core/packet-server/keeper/keeper.go +++ b/modules/core/packet-server/keeper/keeper.go @@ -115,9 +115,9 @@ func (k Keeper) RecvPacket( packet channeltypes.Packet, proof []byte, proofHeight exported.Height, -) error { +) (string, error) { if packet.ProtocolVersion != channeltypes.IBC_VERSION_2 { - return channeltypes.ErrInvalidPacket + return "", channeltypes.ErrInvalidPacket } // Lookup counterparty associated with our channel and ensure that it was packet was indeed @@ -125,17 +125,17 @@ func (k Keeper) RecvPacket( // Note: This can be implemented by the current keeper counterparty, ok := k.ClientKeeper.GetCounterparty(ctx, packet.DestinationChannel) if !ok { - return channeltypes.ErrChannelNotFound + return "", channeltypes.ErrChannelNotFound } if counterparty.ClientId != packet.SourceChannel { - return channeltypes.ErrInvalidChannelIdentifier + return "", channeltypes.ErrInvalidChannelIdentifier } // check if packet timed out by comparing it with the latest height of the chain selfHeight, selfTimestamp := clienttypes.GetSelfHeight(ctx), uint64(ctx.BlockTime().UnixNano()) timeout := channeltypes.NewTimeout(packet.GetTimeoutHeight().(clienttypes.Height), packet.GetTimeoutTimestamp()) if timeout.Elapsed(selfHeight, selfTimestamp) { - return errorsmod.Wrap(timeout.ErrTimeoutElapsed(selfHeight, selfTimestamp), "packet timeout elapsed") + return "", errorsmod.Wrap(timeout.ErrTimeoutElapsed(selfHeight, selfTimestamp), "packet timeout elapsed") } // REPLAY PROTECTION: Packet receipts will indicate that a packet has already been received @@ -147,7 +147,7 @@ func (k Keeper) RecvPacket( // This error indicates that the packet has already been relayed. Core IBC will // treat this error as a no-op in order to prevent an entire relay transaction // from failing and consuming unnecessary fees. - return channeltypes.ErrNoOpMsg + return "", channeltypes.ErrNoOpMsg } // create key/value pair for proof verification by appending the ICS24 path to the last element of the counterparty merklepath @@ -166,7 +166,7 @@ func (k Keeper) RecvPacket( merklePath, commitment, ); err != nil { - return err + return "", err } // Set Packet Receipt to prevent timeout from occurring on counterparty @@ -178,7 +178,7 @@ func (k Keeper) RecvPacket( // emit the same events as receive packet without channel fields channelkeeper.EmitRecvPacketEvent(ctx, packet, sentinelChannel(packet.DestinationChannel)) - return nil + return packet.AppVersion, nil } func (k Keeper) WriteAcknowledgement( @@ -243,20 +243,20 @@ func (k Keeper) AcknowledgePacket( acknowledgement []byte, proofAcked []byte, proofHeight exported.Height, -) error { +) (string, error) { if packet.ProtocolVersion != channeltypes.IBC_VERSION_2 { - return channeltypes.ErrInvalidPacket + return "", channeltypes.ErrInvalidPacket } // Lookup counterparty associated with our channel and ensure that it was packet was indeed // sent by our counterparty. counterparty, ok := k.ClientKeeper.GetCounterparty(ctx, packet.SourceChannel) if !ok { - return channeltypes.ErrChannelNotFound + return "", channeltypes.ErrChannelNotFound } if counterparty.ClientId != packet.DestinationChannel { - return channeltypes.ErrInvalidChannelIdentifier + return "", channeltypes.ErrInvalidChannelIdentifier } commitment := k.ChannelKeeper.GetPacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) @@ -267,14 +267,14 @@ func (k Keeper) AcknowledgePacket( // or there is a misconfigured relayer attempting to prove an acknowledgement // for a packet never sent. Core IBC will treat this error as a no-op in order to // prevent an entire relay transaction from failing and consuming unnecessary fees. - return channeltypes.ErrNoOpMsg + return "", channeltypes.ErrNoOpMsg } packetCommitment := channeltypes.CommitPacket(packet) // verify we sent the packet and haven't cleared it out yet if !bytes.Equal(commitment, packetCommitment) { - return errorsmod.Wrapf(channeltypes.ErrInvalidPacket, "commitment bytes are not equal: got (%v), expected (%v)", packetCommitment, commitment) + return "", errorsmod.Wrapf(channeltypes.ErrInvalidPacket, "commitment bytes are not equal: got (%v), expected (%v)", packetCommitment, commitment) } path := host.PacketAcknowledgementKey(packet.DestinationPort, packet.DestinationChannel, packet.Sequence) @@ -289,7 +289,7 @@ func (k Keeper) AcknowledgePacket( merklePath, channeltypes.CommitAcknowledgement(acknowledgement), ); err != nil { - return err + return "", err } k.ChannelKeeper.DeletePacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) @@ -300,39 +300,40 @@ func (k Keeper) AcknowledgePacket( // emit the same events as acknowledge packet without channel fields channelkeeper.EmitAcknowledgePacketEvent(ctx, packet, sentinelChannel(packet.SourceChannel)) - return nil + return packet.AppVersion, nil } func (k Keeper) TimeoutPacket( ctx sdk.Context, + _ *capabilitytypes.Capability, packet channeltypes.Packet, proof []byte, proofHeight exported.Height, _ uint64, -) error { +) (string, error) { if packet.ProtocolVersion != channeltypes.IBC_VERSION_2 { - return channeltypes.ErrInvalidPacket + return "", channeltypes.ErrInvalidPacket } // Lookup counterparty associated with our channel and ensure that destination channel // is the expected counterparty counterparty, ok := k.ClientKeeper.GetCounterparty(ctx, packet.SourceChannel) if !ok { - return channeltypes.ErrChannelNotFound + return "", channeltypes.ErrChannelNotFound } if counterparty.ClientId != packet.DestinationChannel { - return channeltypes.ErrInvalidChannelIdentifier + return "", channeltypes.ErrInvalidChannelIdentifier } // check that timeout height or timeout timestamp has passed on the other end proofTimestamp, err := k.ClientKeeper.GetClientTimestampAtHeight(ctx, packet.SourceChannel, proofHeight) if err != nil { - return err + return "", err } timeout := channeltypes.NewTimeout(packet.GetTimeoutHeight().(clienttypes.Height), packet.GetTimeoutTimestamp()) if !timeout.Elapsed(proofHeight.(clienttypes.Height), proofTimestamp) { - return errorsmod.Wrap(timeout.ErrTimeoutNotReached(proofHeight.(clienttypes.Height), proofTimestamp), "packet timeout not reached") + return "", errorsmod.Wrap(timeout.ErrTimeoutNotReached(proofHeight.(clienttypes.Height), proofTimestamp), "packet timeout not reached") } // check that the commitment has not been cleared and that it matches the packet sent by relayer @@ -344,13 +345,13 @@ func (k Keeper) TimeoutPacket( // or there is a misconfigured relayer attempting to prove a timeout // for a packet never sent. Core IBC will treat this error as a no-op in order to // prevent an entire relay transaction from failing and consuming unnecessary fees. - return channeltypes.ErrNoOpMsg + return "", channeltypes.ErrNoOpMsg } packetCommitment := channeltypes.CommitPacket(packet) // verify we sent the packet and haven't cleared it out yet if !bytes.Equal(commitment, packetCommitment) { - return errorsmod.Wrapf(channeltypes.ErrInvalidPacket, "packet commitment bytes are not equal: got (%v), expected (%v)", commitment, packetCommitment) + return "", errorsmod.Wrapf(channeltypes.ErrInvalidPacket, "packet commitment bytes are not equal: got (%v), expected (%v)", commitment, packetCommitment) } // verify packet receipt absence @@ -365,7 +366,7 @@ func (k Keeper) TimeoutPacket( proof, merklePath, ); err != nil { - return errorsmod.Wrapf(err, "failed packet receipt absence verification for client (%s)", packet.SourceChannel) + return "", errorsmod.Wrapf(err, "failed packet receipt absence verification for client (%s)", packet.SourceChannel) } // delete packet commitment to prevent replay @@ -377,7 +378,7 @@ func (k Keeper) TimeoutPacket( // emit timeout events channelkeeper.EmitTimeoutPacketEvent(ctx, packet, sentinelChannel(packet.SourceChannel)) - return nil + return packet.AppVersion, nil } // sentinelChannel creates a sentinel channel for use in events for Eureka protocol handlers. diff --git a/modules/core/packet-server/keeper/keeper_test.go b/modules/core/packet-server/keeper/keeper_test.go index 027301a9ee4..513a5412dbb 100644 --- a/modules/core/packet-server/keeper/keeper_test.go +++ b/modules/core/packet-server/keeper/keeper_test.go @@ -190,13 +190,11 @@ func (suite *KeeperTestSuite) TestRecvPacket() { path = ibctesting.NewPath(suite.chainA, suite.chainB) path.SetupV2() - packet = channeltypes.NewPacketWithVersion(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ClientID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ClientID, defaultTimeoutHeight, disabledTimeoutTimestamp, "") - - // For now, set packet commitment on A for each case and update clients. Use SendPacket after 7048. - suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), packet.SourcePort, packet.SourceChannel, packet.Sequence, channeltypes.CommitPacket(packet)) + // send packet + sequence, err := path.EndpointA.SendPacketV2(defaultTimeoutHeight, disabledTimeoutTimestamp, "", ibctesting.MockPacketData) + suite.Require().NoError(err) - suite.coordinator.CommitBlock(path.EndpointA.Chain) - suite.Require().NoError(path.EndpointB.UpdateClient()) + packet = channeltypes.NewPacketWithVersion(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ClientID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ClientID, defaultTimeoutHeight, disabledTimeoutTimestamp, "") tc.malleate() @@ -204,7 +202,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() { packetKey := host.PacketCommitmentKey(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) proof, proofHeight := path.EndpointA.QueryProof(packetKey) - err := suite.chainB.App.GetPacketServer().RecvPacket(suite.chainB.GetContext(), nil, packet, proof, proofHeight) + _, err = suite.chainB.App.GetPacketServer().RecvPacket(suite.chainB.GetContext(), nil, packet, proof, proofHeight) expPass := tc.expError == nil if expPass { @@ -323,7 +321,7 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() { func (suite *KeeperTestSuite) TestAcknowledgePacket() { var ( packet channeltypes.Packet - ack exported.Acknowledgement + ack = mock.MockAcknowledgement freezeClient bool ) @@ -398,45 +396,25 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { freezeClient = false - // create packet receipt and acknowledgement - packet = channeltypes.NewPacketWithVersion(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ClientID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ClientID, defaultTimeoutHeight, disabledTimeoutTimestamp, "") // send packet - _, err := suite.chainA.App.GetPacketServer().SendPacket(suite.chainA.GetContext(), nil, packet.SourceChannel, packet.SourcePort, packet.DestinationPort, packet.TimeoutHeight, packet.TimeoutTimestamp, packet.AppVersion, packet.Data) + sequence, err := path.EndpointA.SendPacketV2(defaultTimeoutHeight, disabledTimeoutTimestamp, "", ibctesting.MockPacketData) suite.Require().NoError(err) - // TODO: Clean up code when msg server handler routes correctly. - - // need to update chainA's client representing chainB to prove missing ack - // commit the changes and update the clients - suite.coordinator.CommitBlock(path.EndpointA.Chain) - suite.Require().NoError(path.EndpointB.UpdateClient()) + packet = channeltypes.NewPacketWithVersion(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ClientID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ClientID, defaultTimeoutHeight, disabledTimeoutTimestamp, "") - // get proof of packet commitment from chainA - packetKey := host.PacketCommitmentKey(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) - proof, proofHeight := path.EndpointA.QueryProof(packetKey) - - err = suite.chainB.App.GetPacketServer().RecvPacket(suite.chainB.GetContext(), nil, packet, proof, proofHeight) - suite.Require().NoError(err) - - ack = mock.MockAcknowledgement - err = suite.chainB.App.GetPacketServer().WriteAcknowledgement(suite.chainB.GetContext(), nil, packet, ack) + err = path.EndpointB.RecvPacket(packet) suite.Require().NoError(err) tc.malleate() - // need to update chainA's client representing chainB to prove missing ack - // commit the changes and update the clients - suite.coordinator.CommitBlock(path.EndpointB.Chain) - suite.Require().NoError(path.EndpointA.UpdateClient()) - - packetKey = host.PacketAcknowledgementKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence()) - proof, proofHeight = path.EndpointB.QueryProof(packetKey) + packetKey := host.PacketAcknowledgementKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence()) + proof, proofHeight := path.EndpointB.QueryProof(packetKey) if freezeClient { path.EndpointA.FreezeClient() } - err = suite.chainA.App.GetPacketServer().AcknowledgePacket(suite.chainA.GetContext(), nil, packet, ack.Acknowledgement(), proof, proofHeight) + _, err = suite.chainA.App.GetPacketServer().AcknowledgePacket(suite.chainA.GetContext(), nil, packet, ack.Acknowledgement(), proof, proofHeight) expPass := tc.expError == nil if expPass { @@ -611,7 +589,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { path.EndpointA.FreezeClient() } - err := suite.chainA.App.GetPacketServer().TimeoutPacket(suite.chainA.GetContext(), packet, proof, proofHeight, 0) + _, err := suite.chainA.App.GetPacketServer().TimeoutPacket(suite.chainA.GetContext(), nil, packet, proof, proofHeight, 0) expPass := tc.expError == nil if expPass { diff --git a/testing/endpoint.go b/testing/endpoint.go index 86af3897678..89b4125fb78 100644 --- a/testing/endpoint.go +++ b/testing/endpoint.go @@ -453,6 +453,33 @@ func (endpoint *Endpoint) ChanCloseInit() error { return endpoint.Chain.sendMsgs(msg) } +// SendPacketV2 sends a packet through the packet server using the associated endpoint +// The counterparty client is updated so proofs can be sent to the counterparty chain. +// The packet sequence generated for the packet to be sent is returned. An error +// is returned if one occurs. +func (endpoint *Endpoint) SendPacketV2( + timeoutHeight clienttypes.Height, + timeoutTimestamp uint64, + version string, + data []byte, +) (uint64, error) { + // no need to send message, acting as a module + sequence, err := endpoint.Chain.App.GetPacketServer().SendPacket(endpoint.Chain.GetContext(), nil, endpoint.ClientID, endpoint.ChannelConfig.PortID, endpoint.Counterparty.ChannelConfig.PortID, timeoutHeight, timeoutTimestamp, version, data) + if err != nil { + return 0, err + } + + // commit changes since no message was sent + endpoint.Chain.Coordinator.CommitBlock(endpoint.Chain) + + err = endpoint.Counterparty.UpdateClient() + if err != nil { + return 0, err + } + + return sequence, nil +} + // SendPacket sends a packet through the channel keeper using the associated endpoint // The counterparty client is updated so proofs can be sent to the counterparty chain. // The packet sequence generated for the packet to be sent is returned. An error