imp(callbacks): remove reconstructed packet logic from 'SendPacket' (#…

…4343)

* style: ran golangci-lint

* imp(callbacks): removed unused packet param from processCallback

* imp(callbacks): removed packet from event functions

* imp(callbacks): removed packet from callbackDataGetter functions

* style(callbacks): reorder func arguments for more consistency

* imp(callbacks_test): using ibcmock.PortID for testing instead of empty string

* style(callbacks): renamed packetData to data in 'GetCallbackData' functions

* fix(callbacks): reordered EmitCallbackEvents parameters during usage

modules/apps/callbacks/callbacks_test.go

@@ -16,11 +16,11 @@ import (
 	dbm "github.com/cometbft/cometbft-db"
 	"github.com/cometbft/cometbft/libs/log"
 
+	simapp "github.com/cosmos/ibc-go/modules/apps/callbacks/testing/simapp"
+	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
 	icacontrollertypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
 	icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
 	feetypes "github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types"
-	simapp "github.com/cosmos/ibc-go/modules/apps/callbacks/testing/simapp"
-	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
 	transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
 	ibctesting "github.com/cosmos/ibc-go/v7/testing"
 	ibcmock "github.com/cosmos/ibc-go/v7/testing/mock"

modules/apps/callbacks/export_test.go

@@ -8,16 +8,15 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
-	channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
 	porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types"
 )
 
 // ProcessCallback is a wrapper around processCallback to allow the function to be directly called in tests.
 func (im IBCMiddleware) ProcessCallback(
-	ctx sdk.Context, packet channeltypes.Packet, callbackType types.CallbackType,
+	ctx sdk.Context, callbackType types.CallbackType,
 	callbackData types.CallbackData, callbackExecutor func(sdk.Context) error,
 ) error {
-	return im.processCallback(ctx, packet, callbackType, callbackData, callbackExecutor)
+	return im.processCallback(ctx, callbackType, callbackData, callbackExecutor)
 }
 
 // GetICS4Wrapper is a getter for the IBCMiddleware's ICS4Wrapper.

modules/apps/callbacks/fee_transfer_test.go

@@ -7,8 +7,8 @@ import (
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
-	feetypes "github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types"
 	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
+	feetypes "github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types"
 	ibctesting "github.com/cosmos/ibc-go/v7/testing"
 )
 

modules/apps/callbacks/ibc_middleware.go

@@ -5,8 +5,8 @@ import (
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
-	capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
 	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
+	capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
 	clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
 	porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types"
@@ -89,11 +89,7 @@ func (im IBCMiddleware) SendPacket(
 		return 0, err
 	}
 
-	// Reconstruct the sent packet. The destination portID and channelID are intentionally left empty as the sender information
-	// is only derived from the source packet information in `GetSourceCallbackData`.
-	reconstructedPacket := channeltypes.NewPacket(data, seq, sourcePort, sourceChannel, "", "", timeoutHeight, timeoutTimestamp)
-
-	callbackData, err := types.GetSourceCallbackData(im.app, reconstructedPacket, ctx.GasMeter().GasRemaining(), im.maxCallbackGas)
+	callbackData, err := types.GetSourceCallbackData(im.app, data, sourcePort, ctx.GasMeter().GasRemaining(), im.maxCallbackGas)
 	// SendPacket is not blocked if the packet does not opt-in to callbacks
 	if err != nil {
 		return seq, nil
@@ -105,13 +101,13 @@ func (im IBCMiddleware) SendPacket(
 		)
 	}
 
-	err = im.processCallback(ctx, reconstructedPacket, types.CallbackTypeSendPacket, callbackData, callbackExecutor)
+	err = im.processCallback(ctx, types.CallbackTypeSendPacket, callbackData, callbackExecutor)
 	// contract keeper is allowed to reject the packet send.
 	if err != nil {
 		return 0, err
 	}
 
-	types.EmitCallbackEvent(ctx, reconstructedPacket, types.CallbackTypeSendPacket, callbackData, nil)
+	types.EmitCallbackEvent(ctx, sourcePort, sourceChannel, seq, types.CallbackTypeSendPacket, callbackData, nil)
 	return seq, nil
 }
 
@@ -131,7 +127,9 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 		return err
 	}
 
-	callbackData, err := types.GetSourceCallbackData(im.app, packet, ctx.GasMeter().GasRemaining(), im.maxCallbackGas)
+	callbackData, err := types.GetSourceCallbackData(
+		im.app, packet.GetData(), packet.GetSourcePort(), ctx.GasMeter().GasRemaining(), im.maxCallbackGas,
+	)
 	// OnAcknowledgementPacket is not blocked if the packet does not opt-in to callbacks
 	if err != nil {
 		return nil
@@ -144,8 +142,11 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
-	err = im.processCallback(ctx, packet, types.CallbackTypeAcknowledgementPacket, callbackData, callbackExecutor)
-	types.EmitCallbackEvent(ctx, packet, types.CallbackTypeAcknowledgementPacket, callbackData, err)
+	err = im.processCallback(ctx, types.CallbackTypeAcknowledgementPacket, callbackData, callbackExecutor)
+	types.EmitCallbackEvent(
+		ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(),
+		types.CallbackTypeAcknowledgementPacket, callbackData, err,
+	)
 
 	return nil
 }
@@ -160,7 +161,9 @@ func (im IBCMiddleware) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Pac
 		return err
 	}
 
-	callbackData, err := types.GetSourceCallbackData(im.app, packet, ctx.GasMeter().GasRemaining(), im.maxCallbackGas)
+	callbackData, err := types.GetSourceCallbackData(
+		im.app, packet.GetData(), packet.GetSourcePort(), ctx.GasMeter().GasRemaining(), im.maxCallbackGas,
+	)
 	// OnTimeoutPacket is not blocked if the packet does not opt-in to callbacks
 	if err != nil {
 		return nil
@@ -171,8 +174,11 @@ func (im IBCMiddleware) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Pac
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
-	err = im.processCallback(ctx, packet, types.CallbackTypeTimeoutPacket, callbackData, callbackExecutor)
-	types.EmitCallbackEvent(ctx, packet, types.CallbackTypeTimeoutPacket, callbackData, err)
+	err = im.processCallback(ctx, types.CallbackTypeTimeoutPacket, callbackData, callbackExecutor)
+	types.EmitCallbackEvent(
+		ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(),
+		types.CallbackTypeTimeoutPacket, callbackData, err,
+	)
 
 	return nil
 }
@@ -191,7 +197,9 @@ func (im IBCMiddleware) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet
 		return ack
 	}
 
-	callbackData, err := types.GetDestCallbackData(im.app, packet, ctx.GasMeter().GasRemaining(), im.maxCallbackGas)
+	callbackData, err := types.GetDestCallbackData(
+		im.app, packet.GetData(), packet.GetSourcePort(), ctx.GasMeter().GasRemaining(), im.maxCallbackGas,
+	)
 	// OnRecvPacket is not blocked if the packet does not opt-in to callbacks
 	if err != nil {
 		return ack
@@ -202,8 +210,11 @@ func (im IBCMiddleware) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
-	err = im.processCallback(ctx, packet, types.CallbackTypeReceivePacket, callbackData, callbackExecutor)
-	types.EmitCallbackEvent(ctx, packet, types.CallbackTypeReceivePacket, callbackData, err)
+	err = im.processCallback(ctx, types.CallbackTypeReceivePacket, callbackData, callbackExecutor)
+	types.EmitCallbackEvent(
+		ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
+		types.CallbackTypeReceivePacket, callbackData, err,
+	)
 
 	return ack
 }
@@ -224,7 +235,9 @@ func (im IBCMiddleware) WriteAcknowledgement(
 		return err
 	}
 
-	callbackData, err := types.GetDestCallbackData(im.app, packet, ctx.GasMeter().GasRemaining(), im.maxCallbackGas)
+	callbackData, err := types.GetDestCallbackData(
+		im.app, packet.GetData(), packet.GetSourcePort(), ctx.GasMeter().GasRemaining(), im.maxCallbackGas,
+	)
 	// WriteAcknowledgement is not blocked if the packet does not opt-in to callbacks
 	if err != nil {
 		return nil
@@ -235,8 +248,11 @@ func (im IBCMiddleware) WriteAcknowledgement(
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
-	err = im.processCallback(ctx, packet, types.CallbackTypeReceivePacket, callbackData, callbackExecutor)
-	types.EmitCallbackEvent(ctx, packet, types.CallbackTypeReceivePacket, callbackData, err)
+	err = im.processCallback(ctx, types.CallbackTypeReceivePacket, callbackData, callbackExecutor)
+	types.EmitCallbackEvent(
+		ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
+		types.CallbackTypeReceivePacket, callbackData, err,
+	)
 
 	return nil
 }
@@ -248,7 +264,7 @@ func (im IBCMiddleware) WriteAcknowledgement(
 //   - the contractExecutor runs out of gas and the relayer has not reserved gas grater than or equal to
 //     CommitGasLimit.
 func (IBCMiddleware) processCallback(
-	ctx sdk.Context, packet ibcexported.PacketI, callbackType types.CallbackType,
+	ctx sdk.Context, callbackType types.CallbackType,
 	callbackData types.CallbackData, callbackExecutor func(sdk.Context) error,
 ) (err error) {
 	cachedCtx, writeFn := ctx.CacheContext()

modules/apps/callbacks/ibc_middleware_test.go

@@ -820,11 +820,6 @@ func (s *CallbacksTestSuite) TestProcessCallback() {
 		s.Run(tc.name, func() {
 			s.SetupMockFeeTest()
 
-			// set mock packet, it is only used in logs and not in callback execution
-			mockPacket := channeltypes.NewPacket(
-				ibcmock.MockPacketData, 1, s.path.EndpointA.ChannelConfig.PortID, s.path.EndpointA.ChannelID,
-				s.path.EndpointB.ChannelConfig.PortID, s.path.EndpointB.ChannelID, clienttypes.NewHeight(0, 100), 0)
-
 			// set a callback data that does not allow retry
 			callbackData = types.CallbackData{
 				CallbackAddress:   s.chainB.SenderAccount.GetAddress().String(),
@@ -853,7 +848,7 @@ func (s *CallbacksTestSuite) TestProcessCallback() {
 			s.Require().True(ok)
 
 			processCallback := func() {
-				err = mockCallbackStack.ProcessCallback(ctx, mockPacket, callbackType, callbackData, callbackExecutor)
+				err = mockCallbackStack.ProcessCallback(ctx, callbackType, callbackData, callbackExecutor)
 			}
 
 			expPass := tc.expValue == nil

modules/apps/callbacks/ica_test.go

@@ -11,10 +11,10 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
 
+	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
 	icacontrollertypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
 	icahosttypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"
 	icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
-	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
 	ibctesting "github.com/cosmos/ibc-go/v7/testing"
 )
 

modules/apps/callbacks/testing/simapp/app.go

@@ -96,6 +96,8 @@ import (
 	abci "github.com/cometbft/cometbft/abci/types"
 	"github.com/cometbft/cometbft/libs/log"
 
+	ibccallbacks "github.com/cosmos/ibc-go/modules/apps/callbacks"
+	simappparams "github.com/cosmos/ibc-go/modules/apps/callbacks/testing/simapp/params"
 	"github.com/cosmos/ibc-go/modules/capability"
 	capabilitykeeper "github.com/cosmos/ibc-go/modules/capability/keeper"
 	capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
@@ -110,8 +112,6 @@ import (
 	ibcfee "github.com/cosmos/ibc-go/v7/modules/apps/29-fee"
 	ibcfeekeeper "github.com/cosmos/ibc-go/v7/modules/apps/29-fee/keeper"
 	ibcfeetypes "github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types"
-	ibccallbacks "github.com/cosmos/ibc-go/modules/apps/callbacks"
-	simappparams "github.com/cosmos/ibc-go/modules/apps/callbacks/testing/simapp/params"
 	transfer "github.com/cosmos/ibc-go/v7/modules/apps/transfer"
 	ibctransferkeeper "github.com/cosmos/ibc-go/v7/modules/apps/transfer/keeper"
 	ibctransfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"

modules/apps/callbacks/types/callbacks.go

@@ -68,17 +68,17 @@ type CallbackData struct {
 // GetSourceCallbackData parses the packet data and returns the source callback data.
 func GetSourceCallbackData(
 	packetDataUnmarshaler porttypes.PacketDataUnmarshaler,
-	packet ibcexported.PacketI, remainingGas uint64, maxGas uint64,
+	data []byte, srcPortID string, remainingGas uint64, maxGas uint64,
 ) (CallbackData, error) {
-	return getCallbackData(packetDataUnmarshaler, packet, remainingGas, maxGas, SourceCallbackKey)
+	return getCallbackData(packetDataUnmarshaler, data, srcPortID, remainingGas, maxGas, SourceCallbackKey)
 }
 
 // GetDestCallbackData parses the packet data and returns the destination callback data.
 func GetDestCallbackData(
 	packetDataUnmarshaler porttypes.PacketDataUnmarshaler,
-	packet ibcexported.PacketI, remainingGas uint64, maxGas uint64,
+	data []byte, srcPortID string, remainingGas, maxGas uint64,
 ) (CallbackData, error) {
-	return getCallbackData(packetDataUnmarshaler, packet, remainingGas, maxGas, DestinationCallbackKey)
+	return getCallbackData(packetDataUnmarshaler, data, srcPortID, remainingGas, maxGas, DestinationCallbackKey)
 }
 
 // getCallbackData parses the packet data and returns the callback data.
@@ -87,16 +87,16 @@ func GetDestCallbackData(
 // address and gas limit from the callback data.
 func getCallbackData(
 	packetDataUnmarshaler porttypes.PacketDataUnmarshaler,
-	packet ibcexported.PacketI, remainingGas,
+	data []byte, srcPortID string, remainingGas,
 	maxGas uint64, callbackKey string,
 ) (CallbackData, error) {
 	// unmarshal packet data
-	unmarshaledData, err := packetDataUnmarshaler.UnmarshalPacketData(packet.GetData())
+	packetData, err := packetDataUnmarshaler.UnmarshalPacketData(data)
 	if err != nil {
 		return CallbackData{}, errorsmod.Wrap(ErrCannotUnmarshalPacketData, err.Error())
 	}
 
-	packetDataProvider, ok := unmarshaledData.(ibcexported.PacketDataProvider)
+	packetDataProvider, ok := packetData.(ibcexported.PacketDataProvider)
 	if !ok {
 		return CallbackData{}, ErrNotPacketDataProvider
 	}
@@ -115,9 +115,9 @@ func getCallbackData(
 	// retrieve packet sender from packet data if possible and if needed
 	var packetSender string
 	if callbackKey == SourceCallbackKey {
-		packetData, ok := unmarshaledData.(ibcexported.PacketData)
+		packetData, ok := packetData.(ibcexported.PacketData)
 		if ok {
-			packetSender = packetData.GetPacketSender(packet.GetSourcePort())
+			packetSender = packetData.GetPacketSender(srcPortID)
 		}
 	}
 

modules/apps/callbacks/types/callbacks_test.go

@@ -10,7 +10,6 @@ import (
 	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
 	transfer "github.com/cosmos/ibc-go/v7/modules/apps/transfer"
 	transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
-	channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
 	porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types"
 	ibctesting "github.com/cosmos/ibc-go/v7/testing"
 	ibcmock "github.com/cosmos/ibc-go/v7/testing/mock"
@@ -278,8 +277,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() {
 
 			tc.malleate()
 
-			testPacket := channeltypes.Packet{Data: packetData}
-			callbackData, err := types.GetCallbackData(packetDataUnmarshaler, testPacket, remainingGas, uint64(1_000_000), callbackKey)
+			callbackData, err := types.GetCallbackData(packetDataUnmarshaler, packetData, ibcmock.PortID, remainingGas, uint64(1_000_000), callbackKey)
 
 			expPass := tc.expError == nil
 			if expPass {
@@ -317,8 +315,7 @@ func (s *CallbacksTypesTestSuite) TestGetSourceCallbackDataTransfer() {
 
 	packetUnmarshaler := transfer.IBCModule{}
 
-	testPacket := channeltypes.Packet{Data: packetDataBytes}
-	callbackData, err := types.GetSourceCallbackData(packetUnmarshaler, testPacket, 2_000_000, 1_000_000)
+	callbackData, err := types.GetSourceCallbackData(packetUnmarshaler, packetDataBytes, ibcmock.PortID, 2_000_000, 1_000_000)
 	s.Require().NoError(err)
 	s.Require().Equal(expCallbackData, callbackData)
 }
@@ -345,8 +342,7 @@ func (s *CallbacksTypesTestSuite) TestGetDestCallbackDataTransfer() {
 
 	packetUnmarshaler := transfer.IBCModule{}
 
-	testPacket := channeltypes.Packet{Data: packetDataBytes}
-	callbackData, err := types.GetDestCallbackData(packetUnmarshaler, testPacket, 2_000_000, 1_000_000)
+	callbackData, err := types.GetDestCallbackData(packetUnmarshaler, packetDataBytes, ibcmock.PortID, 2_000_000, 1_000_000)
 	s.Require().NoError(err)
 	s.Require().Equal(expCallbackData, callbackData)
 }

modules/apps/callbacks/types/events.go

@@ -4,8 +4,6 @@ import (
 	"fmt"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
-
-	ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported"
 )
 
 const (
@@ -54,7 +52,9 @@ const (
 // EmitCallbackEvent emits an event for a callback
 func EmitCallbackEvent(
 	ctx sdk.Context,
-	packet ibcexported.PacketI,
+	portID,
+	channelID string,
+	sequence uint64,
 	callbackType CallbackType,
 	callbackData CallbackData,
 	err error,
@@ -65,7 +65,7 @@ func EmitCallbackEvent(
 		sdk.NewAttribute(AttributeKeyCallbackAddress, callbackData.CallbackAddress),
 		sdk.NewAttribute(AttributeKeyCallbackGasLimit, fmt.Sprintf("%d", callbackData.ExecutionGasLimit)),
 		sdk.NewAttribute(AttributeKeyCallbackCommitGasLimit, fmt.Sprintf("%d", callbackData.CommitGasLimit)),
-		sdk.NewAttribute(AttributeKeyCallbackSequence, fmt.Sprintf("%d", packet.GetSequence())),
+		sdk.NewAttribute(AttributeKeyCallbackSequence, fmt.Sprintf("%d", sequence)),
 	}
 	if err == nil {
 		attributes = append(attributes, sdk.NewAttribute(AttributeKeyCallbackResult, AttributeValueCallbackSuccess))
@@ -82,14 +82,14 @@ func EmitCallbackEvent(
 	case CallbackTypeReceivePacket:
 		eventType = EventTypeDestinationCallback
 		attributes = append(
-			attributes, sdk.NewAttribute(AttributeKeyCallbackDestPortID, packet.GetDestPort()),
-			sdk.NewAttribute(AttributeKeyCallbackDestChannelID, packet.GetDestChannel()),
+			attributes, sdk.NewAttribute(AttributeKeyCallbackDestPortID, portID),
+			sdk.NewAttribute(AttributeKeyCallbackDestChannelID, channelID),
 		)
 	default:
 		eventType = EventTypeSourceCallback
 		attributes = append(
-			attributes, sdk.NewAttribute(AttributeKeyCallbackSourcePortID, packet.GetSourcePort()),
-			sdk.NewAttribute(AttributeKeyCallbackSourceChannelID, packet.GetSourceChannel()),
+			attributes, sdk.NewAttribute(AttributeKeyCallbackSourcePortID, portID),
+			sdk.NewAttribute(AttributeKeyCallbackSourceChannelID, channelID),
 		)
 	}