diff --git a/CHANGELOG.md b/CHANGELOG.md index 696f7064bcb..9ac12d0771f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,6 +93,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking * (light-clients/06-solomachine) [\#6313](https://github.com/cosmos/ibc-go/pull/6313) Fix: No-op to avoid panicking on `UpdateState` for invalid misbehaviour submissions. +* (apps/callbacks) [\#8014](https://github.com/cosmos/ibc-go/pull/8014) Callbacks will now return an error acknowledgement if the recvPacket callback fails. This reverts all app callback changes whereas before we only reverted the callback changes. We also error on all callbacks if the callback data is set but malformed whereas before we ignored the error and continued processing. ### Improvements diff --git a/docs/architecture/adr-008-app-caller-cbs.md b/docs/architecture/adr-008-app-caller-cbs.md index 71e7261d45a..585cd144c76 100644 --- a/docs/architecture/adr-008-app-caller-cbs.md +++ b/docs/architecture/adr-008-app-caller-cbs.md @@ -5,6 +5,7 @@ - 2022-08-10: Initial Draft - 2023-03-22: Merged - 2023-09-13: Updated with decisions made in implementation +- 2025-02-24: RecvPacket callback error now returns error acknowledgement ## Status @@ -245,10 +246,12 @@ type ContractKeeper interface { version string, ) error // IBCReceivePacketCallback is called in the destination chain when a packet acknowledgement is written. - // The contract is expected to handle the callback within the user defined gas limit, and handle any errors, - // out of gas, or panics gracefully. - // This entry point is called with a cached context. If an error is returned, then the changes in - // this context will not be persisted, but the packet lifecycle will not be blocked. + // The contract is expected to handle the callback within the user defined gas limit. + // This entry point is called with a cached context. If an error is returned, then the error + // will be written as an error acknowledgement. This will cause the context changes made by the contract + // to be reverted along with any state changes made by the underlying application. + // The error acknowledgement will then be relayed to the sending application which can perform + // its error acknowledgement logic (e.g. refunding tokens back to user) // // The version provided is the base application version for the given packet send. This allows // contracts to determine how to unmarshal the packetData. @@ -397,6 +400,9 @@ func (im IBCMiddleware) OnRecvPacket( ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), types.CallbackTypeReceivePacket, callbackData, err, ) + if err != nil { + return channeltypes.NewErrorAcknowledgement(err) + } return ack } diff --git a/modules/apps/callbacks/callbacks_test.go b/modules/apps/callbacks/callbacks_test.go index 1ede7a17201..c9eb2505e4f 100644 --- a/modules/apps/callbacks/callbacks_test.go +++ b/modules/apps/callbacks/callbacks_test.go @@ -229,6 +229,7 @@ func GetExpectedEvent( ) (abci.Event, bool) { var ( callbackData types.CallbackData + isCbPacket bool err error ) @@ -238,12 +239,12 @@ func GetExpectedEvent( if callbackType == types.CallbackTypeReceivePacket { packet := channeltypes.NewPacket(data, seq, "", "", eventPortID, eventChannelID, clienttypes.ZeroHeight(), 0) - callbackData, err = types.GetDestCallbackData(ctx, packetDataUnmarshaler, packet, maxCallbackGas) + callbackData, isCbPacket, err = types.GetDestCallbackData(ctx, packetDataUnmarshaler, packet, maxCallbackGas) } else { packet := channeltypes.NewPacket(data, seq, eventPortID, eventChannelID, "", "", clienttypes.ZeroHeight(), 0) - callbackData, err = types.GetSourceCallbackData(ctx, packetDataUnmarshaler, packet, maxCallbackGas) + callbackData, isCbPacket, err = types.GetSourceCallbackData(ctx, packetDataUnmarshaler, packet, maxCallbackGas) } - if err != nil { + if !isCbPacket || err != nil { return abci.Event{}, false } diff --git a/modules/apps/callbacks/ibc_middleware.go b/modules/apps/callbacks/ibc_middleware.go index 32f610e8848..d11872d88c9 100644 --- a/modules/apps/callbacks/ibc_middleware.go +++ b/modules/apps/callbacks/ibc_middleware.go @@ -100,11 +100,16 @@ func (im IBCMiddleware) SendPacket( // packet is created without destination information present, GetSourceCallbackData does not use these. packet := channeltypes.NewPacket(data, seq, sourcePort, sourceChannel, "", "", timeoutHeight, timeoutTimestamp) - callbackData, err := types.GetSourceCallbackData(ctx, im.app, packet, im.maxCallbackGas) + callbackData, isCbPacket, err := types.GetSourceCallbackData(ctx, im.app, packet, im.maxCallbackGas) // SendPacket is not blocked if the packet does not opt-in to callbacks - if err != nil { + if !isCbPacket { return seq, nil } + // if the packet does opt-in to callbacks but the callback data is malformed, + // then the packet send is rejected. + if err != nil { + return 0, err + } callbackExecutor := func(cachedCtx sdk.Context) error { return im.contractKeeper.IBCSendPacketCallback( @@ -139,13 +144,19 @@ func (im IBCMiddleware) OnAcknowledgementPacket( return err } - callbackData, err := types.GetSourceCallbackData( + callbackData, isCbPacket, err := types.GetSourceCallbackData( ctx, im.app, packet, im.maxCallbackGas, ) // OnAcknowledgementPacket is not blocked if the packet does not opt-in to callbacks - if err != nil { + if !isCbPacket { return nil } + // if the packet does opt-in to callbacks but the callback data is malformed, + // then the packet acknowledgement is rejected. + // This should never occur, since this error is already checked on `SendPacket` + if err != nil { + return err + } callbackExecutor := func(cachedCtx sdk.Context) error { return im.contractKeeper.IBCOnAcknowledgementPacketCallback( @@ -173,13 +184,19 @@ func (im IBCMiddleware) OnTimeoutPacket(ctx sdk.Context, channelVersion string, return err } - callbackData, err := types.GetSourceCallbackData( + callbackData, isCbPacket, err := types.GetSourceCallbackData( ctx, im.app, packet, im.maxCallbackGas, ) // OnTimeoutPacket is not blocked if the packet does not opt-in to callbacks - if err != nil { + if !isCbPacket { return nil } + // if the packet does opt-in to callbacks but the callback data is malformed, + // then the packet timeout is rejected. + // This should never occur, since this error is already checked on `SendPacket` + if err != nil { + return err + } callbackExecutor := func(cachedCtx sdk.Context) error { return im.contractKeeper.IBCOnTimeoutPacketCallback(cachedCtx, packet, relayer, callbackData.CallbackAddress, callbackData.SenderAddress, callbackData.ApplicationVersion) @@ -209,24 +226,35 @@ func (im IBCMiddleware) OnRecvPacket(ctx sdk.Context, channelVersion string, pac return ack } - callbackData, err := types.GetDestCallbackData( + callbackData, isCbPacket, err := types.GetDestCallbackData( ctx, im.app, packet, im.maxCallbackGas, ) // OnRecvPacket is not blocked if the packet does not opt-in to callbacks - if err != nil { + if !isCbPacket { return ack } + // if the packet does opt-in to callbacks but the callback data is malformed, + // then the packet receive is rejected. + if err != nil { + return channeltypes.NewErrorAcknowledgement(err) + } callbackExecutor := func(cachedCtx sdk.Context) error { return im.contractKeeper.IBCReceivePacketCallback(cachedCtx, packet, ack, callbackData.CallbackAddress, callbackData.ApplicationVersion) } - // callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions + // callback execution errors in RecvPacket are allowed to write an error acknowledgement + // in this case, the receive logic of the underlying app is reverted + // and the error acknowledgement is processed on the sending chain + // Thus the sending application MUST be capable of processing the standard channel acknowledgement err = internal.ProcessCallback(ctx, types.CallbackTypeReceivePacket, callbackData, callbackExecutor) types.EmitCallbackEvent( ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), types.CallbackTypeReceivePacket, callbackData, err, ) + if err != nil { + return channeltypes.NewErrorAcknowledgement(err) + } return ack } @@ -251,13 +279,17 @@ func (im IBCMiddleware) WriteAcknowledgement( panic(fmt.Errorf("expected type %T, got %T", &channeltypes.Packet{}, packet)) } - callbackData, err := types.GetDestCallbackData( + callbackData, isCbPacket, err := types.GetDestCallbackData( ctx, im.app, chanPacket, im.maxCallbackGas, ) // WriteAcknowledgement is not blocked if the packet does not opt-in to callbacks - if err != nil { + if !isCbPacket { return nil } + // This should never occur, since this error is already checked on `OnRecvPacket` + if err != nil { + return err + } callbackExecutor := func(cachedCtx sdk.Context) error { return im.contractKeeper.IBCReceivePacketCallback(cachedCtx, packet, ack, callbackData.CallbackAddress, callbackData.ApplicationVersion) diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go index 739e0ec43bc..9ffc6627ab8 100644 --- a/modules/apps/callbacks/ibc_middleware_test.go +++ b/modules/apps/callbacks/ibc_middleware_test.go @@ -110,14 +110,24 @@ func (s *CallbacksTestSuite) TestSendPacket() { nil, }, { - "success: no-op on callback data is not valid", + "success: callback data doesn't exist", + func() { + //nolint:goconst + packetData.Memo = "" + }, + "none", // nonexistent callback data should result in no callback execution + false, + nil, + }, + { + "failure: callback data is not valid", func() { //nolint:goconst packetData.Memo = `{"src_callback": {"address": ""}}` }, "none", // improperly formatted callback data should result in no callback execution false, - nil, + types.ErrCallbackAddressNotFound, }, { "failure: ics4Wrapper SendPacket call fails", @@ -244,6 +254,16 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() { callbackSuccess, nil, }, + { + "success: callback data doesn't exist", + func() { + //nolint:goconst + packetData.Memo = "" + packet.Data = packetData.GetBytes() + }, + noExecution, + nil, + }, { "failure: underlying app OnAcknowledgePacket fails", func() { @@ -253,14 +273,14 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() { ibcerrors.ErrUnknownRequest, }, { - "success: no-op on callback data is not valid", + "failure: no-op on callback data is not valid", func() { //nolint:goconst packetData.Memo = `{"src_callback": {"address": ""}}` packet.Data = packetData.GetBytes() }, noExecution, - nil, + types.ErrCallbackAddressNotFound, }, { "failure: callback execution reach out of gas, but sufficient gas provided by relayer", @@ -405,6 +425,16 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() { callbackSuccess, nil, }, + { + "success: callback data doesn't exist", + func() { + //nolint:goconst + packetData.Memo = "" + packet.Data = packetData.GetBytes() + }, + noExecution, + nil, + }, { "failure: underlying app OnTimeoutPacket fails", func() { @@ -414,14 +444,14 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() { ibcerrors.ErrInvalidType, }, { - "success: no-op on callback data is not valid", + "failure: no-op on callback data is not valid", func() { //nolint:goconst packetData.Memo = `{"src_callback": {"address": ""}}` packet.Data = packetData.GetBytes() }, noExecution, - nil, + types.ErrCallbackAddressNotFound, }, { "failure: callback execution reach out of gas, but sufficient gas provided by relayer", @@ -571,6 +601,16 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() { callbackSuccess, successAck, }, + { + "success: callback data doesn't exist", + func() { + //nolint:goconst + packetData.Memo = "" + packet.Data = packetData.GetBytes() + }, + noExecution, + successAck, + }, { "failure: underlying app OnRecvPacket fails", func() { @@ -580,14 +620,14 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() { channeltypes.NewErrorAcknowledgement(ibcerrors.ErrInvalidType), }, { - "success: no-op on callback data is not valid", + "failure: callback data is not valid", func() { //nolint:goconst packetData.Memo = `{"dest_callback": {"address": ""}}` packet.Data = packetData.GetBytes() }, noExecution, - successAck, + channeltypes.NewErrorAcknowledgement(types.ErrCallbackAddressNotFound), }, { "failure: callback execution reach out of gas, but sufficient gas provided by relayer", @@ -730,13 +770,23 @@ func (s *CallbacksTestSuite) TestWriteAcknowledgement() { nil, }, { - "success: no-op on callback data is not valid", + "success: callback data doesn't exist", + func() { + //nolint:goconst + packetData.Memo = "" + packet.Data = packetData.GetBytes() + }, + "none", // nonexistent callback data should result in no callback execution + nil, + }, + { + "failure: callback data is not valid", func() { packetData.Memo = `{"dest_callback": {"address": ""}}` packet.Data = packetData.GetBytes() }, "none", // improperly formatted callback data should result in no callback execution - nil, + types.ErrCallbackAddressNotFound, }, { "failure: ics4Wrapper WriteAcknowledgement call fails", diff --git a/modules/apps/callbacks/replay_test.go b/modules/apps/callbacks/replay_test.go index 79c4da5ac86..379e8515e17 100644 --- a/modules/apps/callbacks/replay_test.go +++ b/modules/apps/callbacks/replay_test.go @@ -226,7 +226,7 @@ func (s *CallbacksTestSuite) TestTransferSuccessAcknowledgementReplayProtection( initialBalance := GetSimApp(s.chainA).BankKeeper.GetBalance(s.chainA.GetContext(), s.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom) // amountTransferred := ibctesting.TestCoin - s.ExecuteTransfer(tc.transferMemo) + s.ExecuteTransfer(tc.transferMemo, true) // check that the callback is executed 1 times s.Require().Equal(1, callbackCount) @@ -290,7 +290,7 @@ func (s *CallbacksTestSuite) TestTransferRecvPacketReplayProtection() { initialBalance := GetSimApp(s.chainB).BankKeeper.GetBalance(s.chainB.GetContext(), s.chainB.SenderAccount.GetAddress(), denom.IBCDenom()) // execute the transfer - s.ExecuteTransfer(tc.transferMemo) + s.ExecuteTransfer(tc.transferMemo, true) // check that the callback is executed 1 times s.Require().Equal(1, callbackCount) diff --git a/modules/apps/callbacks/transfer_test.go b/modules/apps/callbacks/transfer_test.go index e21ab8903e3..3a40a0c1ecb 100644 --- a/modules/apps/callbacks/transfer_test.go +++ b/modules/apps/callbacks/transfer_test.go @@ -11,6 +11,7 @@ import ( "github.com/cosmos/ibc-go/modules/apps/callbacks/types" transfertypes "github.com/cosmos/ibc-go/v10/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v10/modules/core/02-client/types" + channeltypes "github.com/cosmos/ibc-go/v10/modules/core/04-channel/types" ibctesting "github.com/cosmos/ibc-go/v10/testing" ) @@ -46,10 +47,10 @@ func (s *CallbacksTestSuite) TestTransferCallbacks() { true, }, { - "success: dest callback with missing address", + "failure: dest callback with missing address", `{"dest_callback": {"address": ""}}`, "none", - true, + false, }, { "success: source callback", @@ -106,7 +107,7 @@ func (s *CallbacksTestSuite) TestTransferCallbacks() { s.Run(tc.name, func() { s.SetupTransferTest() - s.ExecuteTransfer(tc.transferMemo) + s.ExecuteTransfer(tc.transferMemo, tc.expSuccess) s.AssertHasExecutedExpectedCallback(tc.expCallback, tc.expSuccess) }) } @@ -177,13 +178,15 @@ func (s *CallbacksTestSuite) TestTransferTimeoutCallbacks() { // ExecuteTransfer executes a transfer message on chainA for ibctesting.TestCoin (100 "stake"). // It checks that the transfer is successful and that the packet is relayed to chainB. -func (s *CallbacksTestSuite) ExecuteTransfer(memo string) { +func (s *CallbacksTestSuite) ExecuteTransfer(memo string, recvSuccess bool) { escrowAddress := transfertypes.GetEscrowAddress(s.path.EndpointA.ChannelConfig.PortID, s.path.EndpointA.ChannelID) // record the balance of the escrow address before the transfer escrowBalance := GetSimApp(s.chainA).BankKeeper.GetBalance(s.chainA.GetContext(), escrowAddress, sdk.DefaultBondDenom) // record the balance of the receiving address before the transfer denom := transfertypes.NewDenom(sdk.DefaultBondDenom, transfertypes.NewHop(s.path.EndpointB.ChannelConfig.PortID, s.path.EndpointB.ChannelID)) receiverBalance := GetSimApp(s.chainB).BankKeeper.GetBalance(s.chainB.GetContext(), s.chainB.SenderAccount.GetAddress(), denom.IBCDenom()) + // record the balance of the sending address before the transfer + senderBalance := GetSimApp(s.chainA).BankKeeper.GetBalance(s.chainA.GetContext(), s.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom) amount := ibctesting.TestCoin msg := transfertypes.NewMsgTransfer( @@ -200,17 +203,43 @@ func (s *CallbacksTestSuite) ExecuteTransfer(memo string) { return // we return if send packet is rejected } + // packet found, relay from A to B + err = s.path.EndpointB.UpdateClient() + s.Require().NoError(err) + packet, err := ibctesting.ParsePacketFromEvents(res.GetEvents()) s.Require().NoError(err) - // relay send - err = s.path.RelayPacket(packet) - s.Require().NoError(err) // relay committed + res, err = s.path.EndpointB.RecvPacketWithResult(packet) + s.Require().NoError(err) + + acknowledgement, err := ibctesting.ParseAckFromEvents(res.Events) + s.Require().NoError(err) - // check that the escrow address balance increased by 100 - s.Require().Equal(escrowBalance.Add(amount), GetSimApp(s.chainA).BankKeeper.GetBalance(s.chainA.GetContext(), escrowAddress, sdk.DefaultBondDenom)) - // check that the receiving address balance increased by 100 - s.Require().Equal(receiverBalance.AddAmount(sdkmath.NewInt(100)), GetSimApp(s.chainB).BankKeeper.GetBalance(s.chainB.GetContext(), s.chainB.SenderAccount.GetAddress(), denom.IBCDenom())) + err = s.path.EndpointA.AcknowledgePacket(packet, acknowledgement) + s.Require().NoError(err) + + var ack channeltypes.Acknowledgement + err = transfertypes.ModuleCdc.UnmarshalJSON(acknowledgement, &ack) + s.Require().NoError(err) + + s.Require().Equal(recvSuccess, ack.Success(), "acknowledgement success is not as expected") + + if recvSuccess { + // check that the escrow address balance increased by 100 + s.Require().Equal(escrowBalance.Add(amount), GetSimApp(s.chainA).BankKeeper.GetBalance(s.chainA.GetContext(), escrowAddress, sdk.DefaultBondDenom)) + // check that the receiving address balance increased by 100 + s.Require().Equal(receiverBalance.AddAmount(sdkmath.NewInt(100)), GetSimApp(s.chainB).BankKeeper.GetBalance(s.chainB.GetContext(), s.chainB.SenderAccount.GetAddress(), denom.IBCDenom())) + // check that the sending address balance decreased by 100 + s.Require().Equal(senderBalance.Sub(amount), GetSimApp(s.chainA).BankKeeper.GetBalance(s.chainA.GetContext(), s.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom)) + } else { + // check that the escrow address balance is the same as before the transfer + s.Require().Equal(escrowBalance, GetSimApp(s.chainA).BankKeeper.GetBalance(s.chainA.GetContext(), escrowAddress, sdk.DefaultBondDenom)) + // check that the receiving address balance is the same as before the transfer + s.Require().Equal(receiverBalance, GetSimApp(s.chainB).BankKeeper.GetBalance(s.chainB.GetContext(), s.chainB.SenderAccount.GetAddress(), denom.IBCDenom())) + // check that the sending address balance is the same as before the transfer + s.Require().Equal(senderBalance, GetSimApp(s.chainA).BankKeeper.GetBalance(s.chainA.GetContext(), s.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom)) + } } // ExecuteTransferTimeout executes a transfer message on chainA for 100 denom. diff --git a/modules/apps/callbacks/types/callbacks.go b/modules/apps/callbacks/types/callbacks.go index 4e767446d0f..55873e2d37a 100644 --- a/modules/apps/callbacks/types/callbacks.go +++ b/modules/apps/callbacks/types/callbacks.go @@ -4,8 +4,6 @@ import ( "strconv" "strings" - errorsmod "cosmossdk.io/errors" - sdk "github.com/cosmos/cosmos-sdk/types" channeltypes "github.com/cosmos/ibc-go/v10/modules/core/04-channel/types" @@ -84,10 +82,10 @@ func GetSourceCallbackData( packetDataUnmarshaler porttypes.PacketDataUnmarshaler, packet channeltypes.Packet, maxGas uint64, -) (CallbackData, error) { +) (CallbackData, bool, error) { packetData, version, err := packetDataUnmarshaler.UnmarshalPacketData(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetData()) if err != nil { - return CallbackData{}, errorsmod.Wrap(ErrCannotUnmarshalPacketData, err.Error()) + return CallbackData{}, false, nil } return GetCallbackData(packetData, version, packet.GetSourcePort(), ctx.GasMeter().GasRemaining(), maxGas, SourceCallbackKey) @@ -98,16 +96,19 @@ func GetDestCallbackData( ctx sdk.Context, packetDataUnmarshaler porttypes.PacketDataUnmarshaler, packet channeltypes.Packet, maxGas uint64, -) (CallbackData, error) { +) (CallbackData, bool, error) { packetData, version, err := packetDataUnmarshaler.UnmarshalPacketData(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetData()) if err != nil { - return CallbackData{}, errorsmod.Wrap(ErrCannotUnmarshalPacketData, err.Error()) + return CallbackData{}, false, nil } return GetCallbackData(packetData, version, packet.GetSourcePort(), ctx.GasMeter().GasRemaining(), maxGas, DestinationCallbackKey) } // GetCallbackData parses the packet data and returns the callback data. +// If the packet data does not have callback data set, it will return false for `isCbPacket` and an error. +// If the packet data does have callback data set but the callback data is malformed, +// it will return true for `isCbPacket` and an error // It also checks that the remaining gas is greater than the gas limit specified in the packet data. // The addressGetter and gasLimitGetter functions are used to retrieve the callback // address and gas limit from the callback data. @@ -116,21 +117,21 @@ func GetCallbackData( version, srcPortID string, remainingGas, maxGas uint64, callbackKey string, -) (CallbackData, error) { +) (cbData CallbackData, isCbPacket bool, err error) { packetDataProvider, ok := packetData.(ibcexported.PacketDataProvider) if !ok { - return CallbackData{}, ErrNotPacketDataProvider + return CallbackData{}, false, ErrNotPacketDataProvider } callbackData, ok := packetDataProvider.GetCustomPacketData(callbackKey).(map[string]interface{}) if callbackData == nil || !ok { - return CallbackData{}, ErrCallbackKeyNotFound + return CallbackData{}, false, ErrCallbackKeyNotFound } // get the callback address from the callback data callbackAddress := getCallbackAddress(callbackData) if strings.TrimSpace(callbackAddress) == "" { - return CallbackData{}, ErrCallbackAddressNotFound + return CallbackData{}, true, ErrCallbackAddressNotFound } // retrieve packet sender from packet data if possible and if needed @@ -151,7 +152,7 @@ func GetCallbackData( SenderAddress: packetSender, CommitGasLimit: commitGasLimit, ApplicationVersion: version, - }, nil + }, true, nil } func computeExecAndCommitGasLimit(callbackData map[string]interface{}, remainingGas, maxGas uint64) (uint64, uint64) { diff --git a/modules/apps/callbacks/types/callbacks_test.go b/modules/apps/callbacks/types/callbacks_test.go index c289e6cbfa3..2158a816c0b 100644 --- a/modules/apps/callbacks/types/callbacks_test.go +++ b/modules/apps/callbacks/types/callbacks_test.go @@ -31,10 +31,11 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { // max gas is 1_000_000 testCases := []struct { - name string - malleate func() - expCallbackData types.CallbackData - expError error + name string + malleate func() + expCallbackData types.CallbackData + expIsCallbackData bool + expError error }{ { "success: source callback", @@ -56,6 +57,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { CommitGasLimit: 1_000_000, ApplicationVersion: transfertypes.V1, }, + true, nil, }, { @@ -80,6 +82,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { CommitGasLimit: 1_000_000, ApplicationVersion: transfertypes.V1, }, + true, nil, }, { @@ -104,6 +107,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { CommitGasLimit: 1_000_000, ApplicationVersion: transfertypes.V1, }, + true, nil, }, { @@ -127,6 +131,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { CommitGasLimit: 50_000, ApplicationVersion: transfertypes.V1, }, + true, nil, }, { @@ -149,6 +154,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { CommitGasLimit: 200_000, ApplicationVersion: transfertypes.V1, }, + true, nil, }, { @@ -171,6 +177,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { CommitGasLimit: 1_000_000, ApplicationVersion: transfertypes.V1, }, + true, nil, }, { @@ -195,6 +202,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { CommitGasLimit: 1_000_000, ApplicationVersion: transfertypes.V1, }, + true, nil, }, { @@ -217,6 +225,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { CommitGasLimit: 1_000_000, ApplicationVersion: transfertypes.V1, }, + true, nil, }, { @@ -225,6 +234,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { packetData = ibcmock.MockPacketData }, types.CallbackData{}, + false, types.ErrNotPacketDataProvider, }, { @@ -240,6 +250,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { } }, types.CallbackData{}, + false, types.ErrCallbackKeyNotFound, }, { @@ -255,6 +266,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { } }, types.CallbackData{}, + true, types.ErrCallbackAddressNotFound, }, { @@ -270,6 +282,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { } }, types.CallbackData{}, + true, types.ErrCallbackAddressNotFound, }, @@ -293,6 +306,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { CommitGasLimit: 1_000_000, ApplicationVersion: transfertypes.V1, }, + true, nil, }, { @@ -317,6 +331,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { CommitGasLimit: 1_000_000, ApplicationVersion: transfertypes.V1, }, + true, nil, }, { @@ -325,6 +340,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { packetData = ibcmock.MockPacketData }, types.CallbackData{}, + false, types.ErrNotPacketDataProvider, }, } @@ -339,7 +355,10 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { tc.malleate() - callbackData, err := types.GetCallbackData(packetData, version, transfertypes.PortID, remainingGas, uint64(1_000_000), callbackKey) + callbackData, isCbPacket, err := types.GetCallbackData(packetData, version, transfertypes.PortID, remainingGas, uint64(1_000_000), callbackKey) + + // check if GetCallbackData correctly determines if callback data is present in the packet data + s.Require().Equal(tc.expIsCallbackData, isCbPacket, tc.name) if tc.expError == nil { s.Require().NoError(err, tc.name) @@ -391,7 +410,7 @@ func (s *CallbacksTypesTestSuite) TestGetDestSourceCallbackDataTransfer() { packetDataUnmarshaler porttypes.PacketDataUnmarshaler, packet channeltypes.Packet, maxGas uint64, - ) (types.CallbackData, error) + ) (types.CallbackData, bool, error) getSrc bool }{ { @@ -461,8 +480,9 @@ func (s *CallbacksTypesTestSuite) TestGetDestSourceCallbackDataTransfer() { } else { packet = channeltypes.NewPacket(packetData.GetBytes(), 0, transfertypes.PortID, s.path.EndpointB.ChannelID, transfertypes.PortID, s.path.EndpointA.ChannelID, clienttypes.ZeroHeight(), 0) } - callbackData, err := tc.callbackFn(ctx, packetUnmarshaler, packet, 1_000_000) + callbackData, isCbPacket, err := tc.callbackFn(ctx, packetUnmarshaler, packet, 1_000_000) s.Require().NoError(err) + s.Require().True(isCbPacket) s.Require().Equal(expCallbackData, callbackData) }) } diff --git a/modules/apps/callbacks/v2/ibc_middleware.go b/modules/apps/callbacks/v2/ibc_middleware.go index 2d1c0034c07..09422432ab8 100644 --- a/modules/apps/callbacks/v2/ibc_middleware.go +++ b/modules/apps/callbacks/v2/ibc_middleware.go @@ -103,14 +103,18 @@ func (im IBCMiddleware) OnSendPacket( return nil } - cbData, err := types.GetCallbackData( + cbData, isCbPacket, err := types.GetCallbackData( packetData, payload.GetVersion(), payload.GetSourcePort(), ctx.GasMeter().GasRemaining(), im.maxCallbackGas, types.SourceCallbackKey, ) // OnSendPacket is not blocked if the packet does not opt-in to callbacks - if err != nil { + if !isCbPacket { return nil } + // OnSendPacket is blocked if the packet opts-in to callbacks but the callback data is invalid + if err != nil { + return err + } callbackExecutor := func(cachedCtx sdk.Context) error { return im.contractKeeper.IBCSendPacketCallback( @@ -155,14 +159,20 @@ func (im IBCMiddleware) OnRecvPacket( return recvResult } - cbData, err := types.GetCallbackData( + cbData, isCbPacket, err := types.GetCallbackData( packetData, payload.GetVersion(), payload.GetDestinationPort(), ctx.GasMeter().GasRemaining(), im.maxCallbackGas, types.DestinationCallbackKey, ) // OnRecvPacket is not blocked if the packet does not opt-in to callbacks - if err != nil { + if !isCbPacket { return recvResult } + // OnRecvPacket is blocked if the packet opts-in to callbacks but the callback data is invalid + if err != nil { + return channeltypesv2.RecvPacketResult{ + Status: channeltypesv2.PacketStatus_Failure, + } + } callbackExecutor := func(cachedCtx sdk.Context) error { // reconstruct a channel v1 packet from the v2 packet @@ -189,6 +199,11 @@ func (im IBCMiddleware) OnRecvPacket( ctx, payload.DestinationPort, destinationClient, sequence, types.CallbackTypeReceivePacket, cbData, err, ) + if err != nil { + return channeltypesv2.RecvPacketResult{ + Status: channeltypesv2.PacketStatus_Failure, + } + } return recvResult } @@ -218,14 +233,19 @@ func (im IBCMiddleware) OnAcknowledgementPacket( return nil } - cbData, err := types.GetCallbackData( + cbData, isCbPacket, err := types.GetCallbackData( packetData, payload.GetVersion(), payload.GetSourcePort(), ctx.GasMeter().GasRemaining(), im.maxCallbackGas, types.SourceCallbackKey, ) // OnAcknowledgementPacket is not blocked if the packet does not opt-in to callbacks - if err != nil { + if !isCbPacket { return nil } + // OnAcknowledgementPacket is blocked if the packet opts-in to callbacks but the callback data is invalid + // This should never occur since this error is already checked `OnSendPacket` + if err != nil { + return err + } callbackExecutor := func(cachedCtx sdk.Context) error { // reconstruct a channel v1 packet from the v2 packet @@ -283,14 +303,19 @@ func (im IBCMiddleware) OnTimeoutPacket( return err } - cbData, err := types.GetCallbackData( + cbData, isCbPacket, err := types.GetCallbackData( packetData, payload.GetVersion(), payload.GetSourcePort(), ctx.GasMeter().GasRemaining(), im.maxCallbackGas, types.SourceCallbackKey, ) // OnTimeoutPacket is not blocked if the packet does not opt-in to callbacks - if err != nil { + if !isCbPacket { return nil } + // OnTimeoutPacket is blocked if the packet opts-in to callbacks but the callback data is invalid + // This should never occur since this error is already checked `OnSendPacket` + if err != nil { + return err + } callbackExecutor := func(cachedCtx sdk.Context) error { // reconstruct a channel v1 packet from the v2 packet @@ -354,14 +379,19 @@ func (im IBCMiddleware) WriteAcknowledgement( return err } - cbData, err := types.GetCallbackData( + cbData, isCbPacket, err := types.GetCallbackData( packetData, payload.GetVersion(), payload.GetDestinationPort(), ctx.GasMeter().GasRemaining(), im.maxCallbackGas, types.DestinationCallbackKey, ) // WriteAcknowledgement is not blocked if the packet does not opt-in to callbacks - if err != nil { + if !isCbPacket { return nil } + // WriteAcknowledgement is blocked if the packet opts-in to callbacks but the callback data is invalid + // This should never occur since this error is already checked `OnRecvPacket` + if err != nil { + return err + } recvResult := channeltypesv2.RecvPacketResult{ Status: channeltypesv2.PacketStatus_Success, diff --git a/modules/apps/callbacks/v2/ibc_middleware_test.go b/modules/apps/callbacks/v2/ibc_middleware_test.go index 565f03e076a..658c96c590f 100644 --- a/modules/apps/callbacks/v2/ibc_middleware_test.go +++ b/modules/apps/callbacks/v2/ibc_middleware_test.go @@ -114,14 +114,24 @@ func (s *CallbacksTestSuite) TestSendPacket() { nil, }, { - "success: no-op on callback data is not valid", + "success: callback data nonexistent", + func() { + //nolint:goconst + packetData.Memo = "" + }, + "none", + false, + nil, + }, + { + "failure: no-op on callback data is not valid", func() { //nolint:goconst packetData.Memo = `{"src_callback": {"address": ""}}` }, "none", // improperly formatted callback data should result in no callback execution false, - nil, + types.ErrCallbackAddressNotFound, }, { "failure: callback execution fails", @@ -240,6 +250,15 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() { callbackSuccess, nil, }, + { + "success: callback data nonexistent", + func() { + //nolint:goconst + packetData.Memo = "" + }, + noExecution, + nil, + }, { "failure: underlying app OnAcknowledgePacket fails", func() { @@ -249,13 +268,13 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() { ibcerrors.ErrUnknownRequest, }, { - "success: no-op on callback data is not valid", + "failure: callback data is not valid", func() { //nolint:goconst packetData.Memo = `{"src_callback": {"address": ""}}` }, noExecution, - nil, + types.ErrCallbackAddressNotFound, }, { "failure: callback execution reach out of gas, but sufficient gas provided by relayer", @@ -391,6 +410,15 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() { callbackSuccess, nil, }, + { + "success: callback data nonexistent", + func() { + //nolint:goconst + packetData.Memo = "" + }, + noExecution, + nil, + }, { "failure: underlying app OnTimeoutPacket fails", func() { @@ -400,13 +428,13 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() { transfertypes.ErrInvalidAmount, }, { - "success: no-op on callback data is not valid", + "failure: callback data is not valid", func() { //nolint:goconst packetData.Memo = `{"src_callback": {"address": ""}}` }, noExecution, - nil, + types.ErrCallbackAddressNotFound, }, { "failure: callback execution reach out of gas, but sufficient gas provided by relayer", @@ -557,6 +585,15 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() { callbackSuccess, success, }, + { + "success: callback data nonexistent", + func() { + //nolint:goconst + packetData.Memo = "" + }, + noExecution, + success, + }, { "failure: underlying app OnRecvPacket fails", func() { @@ -566,13 +603,13 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() { failure, }, { - "success: no-op on callback data is not valid", + "failure: no-op on callback data is not valid", func() { //nolint:goconst packetData.Memo = `{"dest_callback": {"address": ""}}` }, noExecution, - success, + failure, }, { "failure: callback execution reach out of gas, but sufficient gas provided by relayer", @@ -580,7 +617,7 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() { packetData.Memo = fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.OogPanicContract, userGasLimit) }, callbackFailed, - success, + failure, }, { "failure: callback execution panics on insufficient gas provided by relayer", @@ -598,7 +635,7 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() { packetData.Memo = fmt.Sprintf(`{"dest_callback": {"address":"%s"}}`, simapp.ErrorContract) }, callbackFailed, - success, + failure, }, } @@ -710,12 +747,21 @@ func (s *CallbacksTestSuite) TestWriteAcknowledgement() { nil, }, { - "success: no-op on callback data is not valid", + "success: callback data nonexistent", + func() { + packetData.Memo = "" + ack = successAck + }, + "none", + nil, + }, + { + "failure: callback data is not valid", func() { packetData.Memo = `{"dest_callback": {"address": ""}}` }, "none", // improperly formatted callback data should result in no callback execution - nil, + types.ErrCallbackAddressNotFound, }, { "failure: ics4Wrapper WriteAcknowledgement call fails", diff --git a/modules/apps/callbacks/v2/v2_test.go b/modules/apps/callbacks/v2/v2_test.go index 26220796335..5c948bbdf49 100644 --- a/modules/apps/callbacks/v2/v2_test.go +++ b/modules/apps/callbacks/v2/v2_test.go @@ -155,8 +155,8 @@ func GetExpectedEvent( if callbackType == types.CallbackTypeReceivePacket { callbackKey = types.DestinationCallbackKey } - callbackData, err := types.GetCallbackData(packetData, version, eventPortID, remainingGas, maxCallbackGas, callbackKey) - if err != nil { + callbackData, isCbPacket, err := types.GetCallbackData(packetData, version, eventPortID, remainingGas, maxCallbackGas, callbackKey) + if !isCbPacket || err != nil { return abci.Event{}, false }