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

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

Merged
merged 8 commits into from
Aug 14, 2023
4 changes: 2 additions & 2 deletions modules/apps/callbacks/callbacks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
5 changes: 2 additions & 3 deletions modules/apps/callbacks/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion modules/apps/callbacks/fee_transfer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down
58 changes: 37 additions & 21 deletions modules/apps/callbacks/ibc_middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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
}

Expand All @@ -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
Expand All @@ -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
}
Expand All @@ -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
Expand All @@ -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
}
Expand All @@ -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
Expand All @@ -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
}
Expand All @@ -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
Expand All @@ -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
}
Expand All @@ -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()
Expand Down
7 changes: 1 addition & 6 deletions modules/apps/callbacks/ibc_middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion modules/apps/callbacks/ica_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down
4 changes: 2 additions & 2 deletions modules/apps/callbacks/testing/simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down
18 changes: 9 additions & 9 deletions modules/apps/callbacks/types/callbacks.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
}
Expand All @@ -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)
}
}

Expand Down
10 changes: 3 additions & 7 deletions modules/apps/callbacks/types/callbacks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down
16 changes: 8 additions & 8 deletions modules/apps/callbacks/types/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import (
"fmt"

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

ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported"
)

const (
Expand Down Expand Up @@ -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,
Expand All @@ -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))
Expand All @@ -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),
)
}

Expand Down
Loading
Loading