move packet executed call to ibc handler (#6312)

* move packet executed to within ibc handler * add spec updates * godoc and nil check to lenght Co-authored-by: Federico Kunze <[email protected]>
cosmos · Jun 4, 2020 · b1f483f · b1f483f
1 parent 50d748b
commit b1f483f
Show file tree

Hide file tree

Showing 6 changed files with 102 additions and 15 deletions.
diff --git a/x/ibc-transfer/module.go b/x/ibc-transfer/module.go
@@ -285,10 +285,10 @@ func (am AppModule) OnChanCloseConfirm(
 func (am AppModule) OnRecvPacket(
 	ctx sdk.Context,
 	packet channeltypes.Packet,
-) (*sdk.Result, error) {
+) (*sdk.Result, []byte, error) {
 	var data FungibleTokenPacketData
 	if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
-		return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error())
+		return nil, nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error())
 	}
 	acknowledgement := FungibleTokenPacketAcknowledgement{
 		Success: true,
@@ -301,10 +301,6 @@ func (am AppModule) OnRecvPacket(
 		}
 	}
 
-	if err := am.keeper.PacketExecuted(ctx, packet, acknowledgement.GetBytes()); err != nil {
-		return nil, err
-	}
-
 	ctx.EventManager().EmitEvent(
 		sdk.NewEvent(
 			EventTypePacket,
@@ -316,7 +312,7 @@ func (am AppModule) OnRecvPacket(
 
 	return &sdk.Result{
 		Events: ctx.EventManager().Events().ToABCIEvents(),
-	}, nil
+	}, acknowledgement.GetBytes(), nil
 }
 
 func (am AppModule) OnAcknowledgementPacket(

diff --git a/x/ibc/04-channel/keeper/packet.go b/x/ibc/04-channel/keeper/packet.go
@@ -210,12 +210,14 @@ func (k Keeper) RecvPacket(
 		return nil, sdkerrors.Wrap(err, "couldn't verify counterparty packet commitment")
 	}
 
+	// NOTE: the remaining code is located in the PacketExecuted function
 	return packet, nil
 }
 
 // PacketExecuted writes the packet execution acknowledgement to the state,
 // which will be verified by the counterparty chain using AcknowledgePacket.
-// CONTRACT: each packet handler function should call WriteAcknowledgement at the end of the execution
+//
+// CONTRACT: this function must be called in the IBC handler
 func (k Keeper) PacketExecuted(
 	ctx sdk.Context,
 	chanCap *capability.Capability,
@@ -237,10 +239,13 @@ func (k Keeper) PacketExecuted(
 
 	capName := host.ChannelCapabilityPath(packet.GetDestPort(), packet.GetDestChannel())
 	if !k.scopedKeeper.AuthenticateCapability(ctx, chanCap, capName) {
-		return sdkerrors.Wrap(types.ErrInvalidChannelCapability, "channel capability failed authentication")
+		return sdkerrors.Wrap(
+			types.ErrInvalidChannelCapability,
+			"channel capability failed authentication",
+		)
 	}
 
-	if acknowledgement != nil || channel.Ordering == types.UNORDERED {
+	if len(acknowledgement) > 0 || channel.Ordering == types.UNORDERED {
 		k.SetPacketAcknowledgement(
 			ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
 			types.CommitAcknowledgement(acknowledgement),
@@ -388,7 +393,7 @@ func (k Keeper) AcknowledgePacket(
 // AcknowledgementExecuted deletes the packet commitment from this chain.
 // It is assumed that the acknowledgement verification has already occurred.
 //
-// NOTE: this function must be called in the handler
+// CONTRACT: this function must be called in the IBC handler
 func (k Keeper) AcknowledgementExecuted(
 	ctx sdk.Context,
 	chanCap *capability.Capability,

diff --git a/x/ibc/04-channel/keeper/timeout.go b/x/ibc/04-channel/keeper/timeout.go
@@ -117,7 +117,7 @@ func (k Keeper) TimeoutPacket(
 // 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.
 //
-// NOTE: this function must be called in the handler
+// CONTRACT: this function must be called in the IBC handler
 func (k Keeper) TimeoutExecuted(
 	ctx sdk.Context,
 	chanCap *capability.Capability,

diff --git a/x/ibc/05-port/types/module.go b/x/ibc/05-port/types/module.go
@@ -58,10 +58,11 @@ type IBCModule interface {
 		channelID string,
 	) error
 
+	// OnRecvPacket must return the acknowledgement bytes
 	OnRecvPacket(
 		ctx sdk.Context,
 		packet channeltypes.Packet,
-	) (*sdk.Result, error)
+	) (*sdk.Result, []byte, error)
 
 	OnAcknowledgementPacket(
 		ctx sdk.Context,

diff --git a/x/ibc/handler.go b/x/ibc/handler.go
@@ -157,7 +157,7 @@ func NewHandler(k Keeper) sdk.Handler {
 		// IBC packet msgs get routed to the appropriate module callback
 		case *channel.MsgPacket:
 			// Lookup module by channel capability
-			module, _, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.DestinationPort, msg.Packet.DestinationChannel)
+			module, cap, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.DestinationPort, msg.Packet.DestinationChannel)
 			if err != nil {
 				return nil, sdkerrors.Wrap(err, "could not retrieve module from port-id")
 			}
@@ -167,7 +167,19 @@ func NewHandler(k Keeper) sdk.Handler {
 			if !ok {
 				return nil, sdkerrors.Wrapf(port.ErrInvalidRoute, "route not found to module: %s", module)
 			}
-			return cbs.OnRecvPacket(ctx, msg.Packet)
+
+			// Perform application logic callback
+			res, ack, err := cbs.OnRecvPacket(ctx, msg.Packet)
+			if err != nil {
+				return nil, err
+			}
+
+			// Set packet acknowledgement
+			if err = k.ChannelKeeper.PacketExecuted(ctx, cap, msg.Packet, ack); err != nil {
+				return nil, err
+			}
+
+			return res, nil
 
 		case *channel.MsgAcknowledgement:
 			// Lookup module by channel capability

diff --git a/x/ibc/spec/05_callbacks.md b/x/ibc/spec/05_callbacks.md
@@ -3,3 +3,76 @@ order: 5
 -->
 
 # Callbacks
+
+Application modules implementing the IBC module must implement the following callbacks as found in [05-port](../05-port/types/module.go).
+
+```go
+// IBCModule defines an interface that implements all the callbacks
+// that modules must define as specified in ICS-26
+type IBCModule interface {
+	OnChanOpenInit(
+		ctx sdk.Context,
+		order channeltypes.Order,
+		connectionHops []string,
+		portID string,
+		channelID string,
+		channelCap *capability.Capability,
+		counterParty channeltypes.Counterparty,
+		version string,
+	) error
+
+	OnChanOpenTry(
+		ctx sdk.Context,
+		order channeltypes.Order,
+		connectionHops []string,
+		portID,
+		channelID string,
+		channelCap *capability.Capability,
+		counterparty channeltypes.Counterparty,
+		version,
+		counterpartyVersion string,
+	) error
+
+	OnChanOpenAck(
+		ctx sdk.Context,
+		portID,
+		channelID string,
+		counterpartyVersion string,
+	) error
+
+	OnChanOpenConfirm(
+		ctx sdk.Context,
+		portID,
+		channelID string,
+	) error
+
+	OnChanCloseInit(
+		ctx sdk.Context,
+		portID,
+		channelID string,
+	) error
+
+	OnChanCloseConfirm(
+		ctx sdk.Context,
+		portID,
+		channelID string,
+	) error
+
+	// OnRecvPacket must return the acknowledgement bytes
+	OnRecvPacket(
+		ctx sdk.Context,
+		packet channeltypes.Packet,
+	) (*sdk.Result, []byte, error)
+
+	OnAcknowledgementPacket(
+		ctx sdk.Context,
+		packet channeltypes.Packet,
+		acknowledgement []byte,
+	) (*sdk.Result, error)
+
+	OnTimeoutPacket(
+		ctx sdk.Context,
+		packet channeltypes.Packet,
+	) (*sdk.Result, error)
+}
+```