From 7b759dd8ac3f7a476453a0c204962ceff03c0f35 Mon Sep 17 00:00:00 2001 From: srdtrk <59252793+srdtrk@users.noreply.github.com> Date: Thu, 24 Aug 2023 10:12:22 +0300 Subject: [PATCH] test(callbacks): simplified mock contract keeper's processCallback logic (#4375) * imp(callbacks/mock/contract_keeeper): improved contract keeper logic * imp(callbacks_test): fixed transfer_test.go * imp(callbacks_test): fixed ica_test.go * imp(callbacks_test): fixed fee_transfer_test.go * style(callbacks_test): removed unneeded gas_limit * style: ran golangci-lint * imp(callbacks_test): implemented simplified logic * imp(callbacks_test): removed unneeded variable * imp(callbacks): implemented some review feedback * docs(callbacks/simapp): updated godocs of mock contract keeper functions --- modules/apps/callbacks/fee_transfer_test.go | 49 ++++++-- modules/apps/callbacks/ibc_middleware_test.go | 93 ++++++++------ modules/apps/callbacks/ica_test.go | 29 +++-- .../testing/simapp/contract_keeper.go | 115 ++++++++++++------ modules/apps/callbacks/transfer_test.go | 51 +++++--- 5 files changed, 226 insertions(+), 111 deletions(-) diff --git a/modules/apps/callbacks/fee_transfer_test.go b/modules/apps/callbacks/fee_transfer_test.go index a0ef977ead5..cb73830e4d8 100644 --- a/modules/apps/callbacks/fee_transfer_test.go +++ b/modules/apps/callbacks/fee_transfer_test.go @@ -7,6 +7,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/ibc-go/modules/apps/callbacks/testing/simapp" "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" @@ -33,19 +34,19 @@ func (s *CallbacksTestSuite) TestIncentivizedTransferCallbacks() { }, { "success: dest callback", - fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.SuccessContract), types.CallbackTypeReceivePacket, true, }, { "success: dest callback with other json fields", - fmt.Sprintf(`{"dest_callback": {"address": "%s"}, "something_else": {}}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}, "something_else": {}}`, simapp.SuccessContract), types.CallbackTypeReceivePacket, true, }, { "success: dest callback with malformed json", - fmt.Sprintf(`{"dest_callback": {"address": "%s"}, malformed}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}, malformed}`, simapp.SuccessContract), "none", true, }, @@ -57,19 +58,19 @@ func (s *CallbacksTestSuite) TestIncentivizedTransferCallbacks() { }, { "success: source callback", - fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract), types.CallbackTypeAcknowledgementPacket, true, }, { "success: source callback with other json fields", - fmt.Sprintf(`{"src_callback": {"address": "%s"}, "something_else": {}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}, "something_else": {}}`, simapp.SuccessContract), types.CallbackTypeAcknowledgementPacket, true, }, { "success: source callback with malformed json", - fmt.Sprintf(`{"src_callback": {"address": "%s"}, malformed}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}, malformed}`, simapp.SuccessContract), "none", true, }, @@ -81,13 +82,25 @@ func (s *CallbacksTestSuite) TestIncentivizedTransferCallbacks() { }, { "failure: dest callback with low gas (panic)", - fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogPanicContract), + types.CallbackTypeReceivePacket, + false, + }, + { + "failure: dest callback with low gas (error)", + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeReceivePacket, false, }, { "failure: source callback with low gas (panic)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract), + types.CallbackTypeSendPacket, + false, + }, + { + "failure: source callback with low gas (error)", + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, @@ -127,25 +140,37 @@ func (s *CallbacksTestSuite) TestIncentivizedTransferTimeoutCallbacks() { }, { "success: dest callback", - fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.SuccessContract), "none", true, // timeouts don't reach destination chain execution }, { "success: source callback", - fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract), types.CallbackTypeTimeoutPacket, true, }, { "success: dest callback with low gas (panic)", - fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogPanicContract), "none", // timeouts don't reach destination chain execution false, }, { "failure: source callback with low gas (panic)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract), + types.CallbackTypeSendPacket, + false, + }, + { + "success: dest callback with low gas (error)", + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogErrorContract), + "none", // timeouts don't reach destination chain execution + false, + }, + { + "failure: source callback with low gas (error)", + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go index f01c9f859a3..c4ad7e9b3a8 100644 --- a/modules/apps/callbacks/ibc_middleware_test.go +++ b/modules/apps/callbacks/ibc_middleware_test.go @@ -127,9 +127,9 @@ func (s *CallbacksTestSuite) TestSendPacket() { channeltypes.ErrChannelNotFound, }, { - "failure: callback execution fails, sender is not callback address", + "failure: callback execution fails", func() { - packetData.Sender = simapp.MockCallbackUnauthorizedAddress + packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s"}}`, simapp.ErrorContract) }, types.CallbackTypeSendPacket, false, @@ -138,11 +138,11 @@ func (s *CallbacksTestSuite) TestSendPacket() { { "failure: callback execution reach out of gas, but sufficient gas provided by relayer", func() { - packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, callbackAddr) + packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, simapp.OogPanicContract) }, types.CallbackTypeSendPacket, true, - sdk.ErrorOutOfGas{Descriptor: fmt.Sprintf("mock %s callback panic", types.CallbackTypeSendPacket)}, + sdk.ErrorOutOfGas{Descriptor: fmt.Sprintf("mock %s callback oog panic", types.CallbackTypeSendPacket)}, }, } @@ -156,8 +156,8 @@ func (s *CallbacksTestSuite) TestSendPacket() { s.Require().True(ok) packetData = transfertypes.NewFungibleTokenPacketData( - ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), callbackAddr, - ibctesting.TestAccAddress, fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, callbackAddr), + ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), ibctesting.TestAccAddress, + ibctesting.TestAccAddress, fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract), ) chanCap := s.path.EndpointA.Chain.GetChannelCapability(s.path.EndpointA.ChannelConfig.PortID, s.path.EndpointA.ChannelID) @@ -200,10 +200,11 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() { ) var ( - packetData transfertypes.FungibleTokenPacketData - packet channeltypes.Packet - ack []byte - ctx sdk.Context + packetData transfertypes.FungibleTokenPacketData + packet channeltypes.Packet + ack []byte + ctx sdk.Context + userGasLimit uint64 ) panicError := fmt.Errorf("panic error") @@ -241,7 +242,7 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() { { "failure: callback execution reach out of gas, but sufficient gas provided by relayer", func() { - packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, callbackAddr) + packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.OogPanicContract, userGasLimit) packet.Data = packetData.GetBytes() }, callbackFailed, @@ -250,15 +251,18 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() { { "failure: callback execution panics on insufficient gas provided by relayer", func() { + packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.OogPanicContract, userGasLimit) + packet.Data = packetData.GetBytes() + ctx = ctx.WithGasMeter(sdk.NewGasMeter(300_000)) }, callbackFailed, panicError, }, { - "failure: callback execution fails, unauthorized address", + "failure: callback execution fails", func() { - packetData.Sender = simapp.MockCallbackUnauthorizedAddress + packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s"}}`, simapp.ErrorContract) packet.Data = packetData.GetBytes() }, callbackFailed, @@ -271,11 +275,10 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() { s.Run(tc.name, func() { s.SetupTransferTest() - // set user gas limit above panic level in mock contract keeper - userGasLimit := 600000 + userGasLimit = 600000 packetData = transfertypes.NewFungibleTokenPacketData( - ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), callbackAddr, ibctesting.TestAccAddress, - fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"%d"}}`, callbackAddr, userGasLimit), + ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), ibctesting.TestAccAddress, ibctesting.TestAccAddress, + fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.SuccessContract, userGasLimit), ) packet = channeltypes.Packet{ @@ -356,13 +359,11 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() { ctx sdk.Context ) - panicError := fmt.Errorf("panic error") - testCases := []struct { name string malleate func() expResult expResult - expError error + expValue interface{} }{ { "success", @@ -391,7 +392,7 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() { { "failure: callback execution reach out of gas, but sufficient gas provided by relayer", func() { - packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, callbackAddr) + packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, simapp.OogPanicContract) packet.Data = packetData.GetBytes() }, callbackFailed, @@ -400,15 +401,20 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() { { "failure: callback execution panics on insufficient gas provided by relayer", func() { + packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s"}}`, simapp.OogPanicContract) + packet.Data = packetData.GetBytes() + ctx = ctx.WithGasMeter(sdk.NewGasMeter(300_000)) }, callbackFailed, - panicError, + sdk.ErrorOutOfGas{ + Descriptor: fmt.Sprintf("ibc %s callback out of gas; commitGasLimit: %d", types.CallbackTypeTimeoutPacket, maxCallbackGas), + }, }, { - "failure: callback execution fails, unauthorized address", + "failure: callback execution fails", func() { - packetData.Sender = simapp.MockCallbackUnauthorizedAddress + packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s"}}`, simapp.ErrorContract) packet.Data = packetData.GetBytes() }, callbackFailed, @@ -455,21 +461,17 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() { return transferStack.OnTimeoutPacket(ctx, packet, s.chainA.SenderAccount.GetAddress()) } - switch tc.expError { + switch expValue := tc.expValue.(type) { case nil: err := onTimeoutPacket() s.Require().Nil(err) - - case panicError: - s.Require().PanicsWithValue(sdk.ErrorOutOfGas{ - Descriptor: fmt.Sprintf("ibc %s callback out of gas; commitGasLimit: %d", types.CallbackTypeTimeoutPacket, userGasLimit), - }, func() { + case error: + err := onTimeoutPacket() + s.Require().ErrorIs(expValue, err) + default: + s.Require().PanicsWithValue(tc.expValue, func() { _ = onTimeoutPacket() }) - - default: - err := onTimeoutPacket() - s.Require().ErrorIs(tc.expError, err) } sourceStatefulCounter := GetSimApp(s.chainA).MockContractKeeper.GetStateEntryCounter(s.chainA.GetContext()) @@ -506,9 +508,10 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() { ) var ( - packetData transfertypes.FungibleTokenPacketData - packet channeltypes.Packet - ctx sdk.Context + packetData transfertypes.FungibleTokenPacketData + packet channeltypes.Packet + ctx sdk.Context + userGasLimit uint64 ) successAck := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) @@ -547,7 +550,7 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() { { "failure: callback execution reach out of gas, but sufficient gas provided by relayer", func() { - packetData.Memo = fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"400000"}}`, callbackAddr) + packetData.Memo = fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.OogPanicContract, userGasLimit) packet.Data = packetData.GetBytes() }, callbackFailed, @@ -556,11 +559,23 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() { { "failure: callback execution panics on insufficient gas provided by relayer", func() { + packetData.Memo = fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.OogPanicContract, userGasLimit) + packet.Data = packetData.GetBytes() + ctx = ctx.WithGasMeter(sdk.NewGasMeter(300_000)) }, callbackFailed, panicAck, }, + { + "failure: callback execution fails", + func() { + packetData.Memo = fmt.Sprintf(`{"dest_callback": {"address":"%s"}}`, simapp.ErrorContract) + packet.Data = packetData.GetBytes() + }, + callbackFailed, + successAck, + }, } for _, tc := range testCases { @@ -569,7 +584,7 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() { s.SetupTransferTest() // set user gas limit above panic level in mock contract keeper - userGasLimit := 600_000 + userGasLimit = 600_000 packetData = transfertypes.NewFungibleTokenPacketData( ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), ibctesting.TestAccAddress, s.chainB.SenderAccount.GetAddress().String(), fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"%d"}}`, ibctesting.TestAccAddress, userGasLimit), diff --git a/modules/apps/callbacks/ica_test.go b/modules/apps/callbacks/ica_test.go index 15f7da8ad1e..48234178730 100644 --- a/modules/apps/callbacks/ica_test.go +++ b/modules/apps/callbacks/ica_test.go @@ -11,6 +11,7 @@ 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/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" icahosttypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types" @@ -34,25 +35,25 @@ func (s *CallbacksTestSuite) TestICACallbacks() { }, { "success: dest callback", - fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.SuccessContract), "none", true, }, { "success: source callback", - fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract), types.CallbackTypeAcknowledgementPacket, true, }, { "success: source callback with other json fields", - fmt.Sprintf(`{"src_callback": {"address": "%s"}, "something_else": {}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}, "something_else": {}}`, simapp.SuccessContract), types.CallbackTypeAcknowledgementPacket, true, }, { "success: source callback with malformed json", - fmt.Sprintf(`{"src_callback": {"address": "%s"}, malformed}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}, malformed}`, simapp.SuccessContract), "none", true, }, @@ -64,7 +65,13 @@ func (s *CallbacksTestSuite) TestICACallbacks() { }, { "failure: source callback with low gas (panic)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "350000"}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract), + types.CallbackTypeSendPacket, + false, + }, + { + "failure: source callback with low gas (error)", + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, @@ -96,19 +103,25 @@ func (s *CallbacksTestSuite) TestICATimeoutCallbacks() { }, { "success: dest callback", - fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.SuccessContract), "none", true, }, { "success: source callback", - fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract), types.CallbackTypeTimeoutPacket, true, }, { "failure: source callback with low gas (panic)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "350000"}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract), + types.CallbackTypeSendPacket, + false, + }, + { + "failure: source callback with low gas (error)", + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, diff --git a/modules/apps/callbacks/testing/simapp/contract_keeper.go b/modules/apps/callbacks/testing/simapp/contract_keeper.go index 0fc16f7a3bf..1e0d9e702f2 100644 --- a/modules/apps/callbacks/testing/simapp/contract_keeper.go +++ b/modules/apps/callbacks/testing/simapp/contract_keeper.go @@ -16,9 +16,19 @@ import ( // MockKeeper implements callbacktypes.ContractKeeper var _ callbacktypes.ContractKeeper = (*ContractKeeper)(nil) -var ( - StatefulCounterKey = "stateful-callback-counter" - MockCallbackUnauthorizedAddress = "cosmos15ulrf36d4wdtrtqzkgaan9ylwuhs7k7qz753uk" +var StatefulCounterKey = "stateful-callback-counter" + +const ( + // OogPanicContract is a contract address that will panic out of gas + OogPanicContract = "panics out of gas" + // OogErrorContract is a contract address that will error out of gas + OogErrorContract = "errors out of gas" + // PanicContract is a contract address that will panic + PanicContract = "panics" + // ErrorContract is a contract address that will return an error + ErrorContract = "errors" + // SuccessContract is a contract address that will return nil + SuccessContract = "success" ) // This is a mock contract keeper used for testing. It is not wired up to any modules. @@ -68,10 +78,13 @@ func NewContractKeeper(key storetypes.StoreKey) ContractKeeper { } } -// IBCPacketSendCallback returns nil if the gas meter has greater than -// or equal to 500_000 gas remaining. -// This function oog panics if the gas remaining is less than 500_000. -// This function errors if the authAddress is MockCallbackUnauthorizedAddress. +// IBCPacketSendCallback increments the stateful entry counter and the send_packet callback counter. +// This function: +// - returns MockApplicationCallbackError and consumes half the remaining gas if the contract address is ErrorContract +// - Oog panics and consumes all the remaining gas + 1 if the contract address is OogPanicContract +// - returns MockApplicationCallbackError and consumes all the remaining gas + 1 if the contract address is OogErrorContract +// - Panics and consumes half the remaining gas if the contract address is PanicContract +// - returns nil and consumes half the remaining gas if the contract address is SuccessContract or any other value func (k ContractKeeper) IBCSendPacketCallback( ctx sdk.Context, sourcePort string, @@ -82,13 +95,16 @@ func (k ContractKeeper) IBCSendPacketCallback( contractAddress, packetSenderAddress string, ) error { - return k.processMockCallback(ctx, callbacktypes.CallbackTypeSendPacket, packetSenderAddress) + return k.processMockCallback(ctx, callbacktypes.CallbackTypeSendPacket, contractAddress) } -// IBCOnAcknowledgementPacketCallback returns nil if the gas meter has greater than -// or equal to 500_000 gas remaining. -// This function oog panics if the gas remaining is less than 500_000. -// This function errors if the authAddress is MockCallbackUnauthorizedAddress. +// IBCOnAcknowledgementPacketCallback increments the stateful entry counter and the acknowledgement_packet callback counter. +// This function: +// - returns MockApplicationCallbackError and consumes half the remaining gas if the contract address is ErrorContract +// - Oog panics and consumes all the remaining gas + 1 if the contract address is OogPanicContract +// - returns MockApplicationCallbackError and consumes all the remaining gas + 1 if the contract address is OogErrorContract +// - Panics and consumes half the remaining gas if the contract address is PanicContract +// - returns nil and consumes half the remaining gas if the contract address is SuccessContract or any other value func (k ContractKeeper) IBCOnAcknowledgementPacketCallback( ctx sdk.Context, packet channeltypes.Packet, @@ -97,13 +113,16 @@ func (k ContractKeeper) IBCOnAcknowledgementPacketCallback( contractAddress, packetSenderAddress string, ) error { - return k.processMockCallback(ctx, callbacktypes.CallbackTypeAcknowledgementPacket, packetSenderAddress) + return k.processMockCallback(ctx, callbacktypes.CallbackTypeAcknowledgementPacket, contractAddress) } -// IBCOnTimeoutPacketCallback returns nil if the gas meter has greater than -// or equal to 500_000 gas remaining. -// This function oog panics if the gas remaining is less than 500_000. -// This function errors if the authAddress is MockCallbackUnauthorizedAddress. +// IBCOnTimeoutPacketCallback increments the stateful entry counter and the timeout_packet callback counter. +// This function: +// - returns MockApplicationCallbackError and consumes half the remaining gas if the contract address is ErrorContract +// - Oog panics and consumes all the remaining gas + 1 if the contract address is OogPanicContract +// - returns MockApplicationCallbackError and consumes all the remaining gas + 1 if the contract address is OogErrorContract +// - Panics and consumes half the remaining gas if the contract address is PanicContract +// - returns nil and consumes half the remaining gas if the contract address is SuccessContract or any other value func (k ContractKeeper) IBCOnTimeoutPacketCallback( ctx sdk.Context, packet channeltypes.Packet, @@ -111,30 +130,38 @@ func (k ContractKeeper) IBCOnTimeoutPacketCallback( contractAddress, packetSenderAddress string, ) error { - return k.processMockCallback(ctx, callbacktypes.CallbackTypeTimeoutPacket, packetSenderAddress) + return k.processMockCallback(ctx, callbacktypes.CallbackTypeTimeoutPacket, contractAddress) } -// IBCReceivePacketCallback returns nil if the gas meter has greater than -// or equal to 500_000 gas remaining. -// This function oog panics if the gas remaining is less than 500_000. -// This function errors if the authAddress is MockCallbackUnauthorizedAddress. +// IBCReceivePacketCallback increments the stateful entry counter and the receive_packet callback counter. +// This function: +// - returns MockApplicationCallbackError and consumes half the remaining gas if the contract address is ErrorContract +// - Oog panics and consumes all the remaining gas + 1 if the contract address is OogPanicContract +// - returns MockApplicationCallbackError and consumes all the remaining gas + 1 if the contract address is OogErrorContract +// - Panics and consumes half the remaining gas if the contract address is PanicContract +// - returns nil and consumes half the remaining gas if the contract address is SuccessContract or any other value func (k ContractKeeper) IBCReceivePacketCallback( ctx sdk.Context, packet ibcexported.PacketI, ack ibcexported.Acknowledgement, contractAddress string, ) error { - return k.processMockCallback(ctx, callbacktypes.CallbackTypeReceivePacket, "") + return k.processMockCallback(ctx, callbacktypes.CallbackTypeReceivePacket, contractAddress) } -// processMockCallback returns nil if the gas meter has greater than or equal to 500_000 gas remaining. -// This function oog panics if the gas remaining is less than 500_000. -// This function errors if the authAddress is MockCallbackUnauthorizedAddress. +// processMockCallback processes a mock callback. +// It increments the stateful entry counter and the callback counter. +// This function: +// - returns MockApplicationCallbackError and consumes half the remaining gas if the contract address is ErrorContract +// - Oog panics and consumes all the remaining gas + 1 if the contract address is OogPanicContract +// - returns MockApplicationCallbackError and consumes all the remaining gas + 1 if the contract address is OogErrorContract +// - Panics and consumes half the remaining gas if the contract address is PanicContract +// - returns nil and consumes half the remaining gas if the contract address is SuccessContract or any other value func (k ContractKeeper) processMockCallback( ctx sdk.Context, callbackType callbacktypes.CallbackType, - authAddress string, -) error { + contractAddress string, +) (err error) { gasRemaining := ctx.GasMeter().GasRemaining() // increment stateful entries, if the callbacks module handler @@ -145,16 +172,28 @@ func (k ContractKeeper) processMockCallback( // increment callback execution attempts k.Counters[callbackType]++ - if gasRemaining < 500000 { - // consume gas will panic since we attempt to consume 500_000 gas, for tests - ctx.GasMeter().ConsumeGas(500000, fmt.Sprintf("mock %s callback panic", callbackType)) - } - - if authAddress == MockCallbackUnauthorizedAddress { - ctx.GasMeter().ConsumeGas(500000, fmt.Sprintf("mock %s callback unauthorized", callbackType)) + switch contractAddress { + case ErrorContract: + // consume half of the remaining gas so that ConsumeGas cannot oog panic + ctx.GasMeter().ConsumeGas(gasRemaining/2, fmt.Sprintf("mock %s callback unauthorized", callbackType)) return ibcmock.MockApplicationCallbackError + case OogPanicContract: + ctx.GasMeter().ConsumeGas(gasRemaining+1, fmt.Sprintf("mock %s callback oog panic", callbackType)) + return nil // unreachable + case OogErrorContract: + defer func() { + _ = recover() + err = ibcmock.MockApplicationCallbackError + }() + ctx.GasMeter().ConsumeGas(gasRemaining+1, fmt.Sprintf("mock %s callback oog error", callbackType)) + return nil // unreachable + case PanicContract: + // consume half of the remaining gas so that ConsumeGas cannot oog panic + ctx.GasMeter().ConsumeGas(gasRemaining/2, fmt.Sprintf("mock %s callback panic", callbackType)) + panic(ibcmock.MockApplicationCallbackError) + default: + // consume half of the remaining gas so that ConsumeGas cannot oog panic + ctx.GasMeter().ConsumeGas(gasRemaining/2, fmt.Sprintf("mock %s callback success", callbackType)) + return nil // success } - - ctx.GasMeter().ConsumeGas(500000, fmt.Sprintf("mock %s callback success", callbackType)) - return nil } diff --git a/modules/apps/callbacks/transfer_test.go b/modules/apps/callbacks/transfer_test.go index e9d4e37a160..e0c4d4e38bd 100644 --- a/modules/apps/callbacks/transfer_test.go +++ b/modules/apps/callbacks/transfer_test.go @@ -5,14 +5,13 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" + "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" clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" ibctesting "github.com/cosmos/ibc-go/v7/testing" ) -var callbackAddr = ibctesting.TestAccAddress - func (s *CallbacksTestSuite) TestTransferCallbacks() { testCases := []struct { name string @@ -28,19 +27,19 @@ func (s *CallbacksTestSuite) TestTransferCallbacks() { }, { "success: dest callback", - fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.SuccessContract), types.CallbackTypeReceivePacket, true, }, { "success: dest callback with other json fields", - fmt.Sprintf(`{"dest_callback": {"address": "%s"}, "something_else": {}}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}, "something_else": {}}`, simapp.SuccessContract), types.CallbackTypeReceivePacket, true, }, { "success: dest callback with malformed json", - fmt.Sprintf(`{"dest_callback": {"address": "%s"}, malformed}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}, malformed}`, simapp.SuccessContract), "none", true, }, @@ -52,19 +51,19 @@ func (s *CallbacksTestSuite) TestTransferCallbacks() { }, { "success: source callback", - fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract), types.CallbackTypeAcknowledgementPacket, true, }, { "success: source callback with other json fields", - fmt.Sprintf(`{"src_callback": {"address": "%s"}, "something_else": {}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}, "something_else": {}}`, simapp.SuccessContract), types.CallbackTypeAcknowledgementPacket, true, }, { "success: source callback with malformed json", - fmt.Sprintf(`{"src_callback": {"address": "%s"}, malformed}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}, malformed}`, simapp.SuccessContract), "none", true, }, @@ -76,13 +75,25 @@ func (s *CallbacksTestSuite) TestTransferCallbacks() { }, { "failure: dest callback with low gas (panic)", - fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogPanicContract), types.CallbackTypeReceivePacket, false, }, { "failure: source callback with low gas (panic)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract), + types.CallbackTypeSendPacket, + false, + }, + { + "failure: dest callback with low gas (error)", + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogErrorContract), + types.CallbackTypeReceivePacket, + false, + }, + { + "failure: source callback with low gas (error)", + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, @@ -111,25 +122,37 @@ func (s *CallbacksTestSuite) TestTransferTimeoutCallbacks() { }, { "success: dest callback", - fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.SuccessContract), "none", // timeouts don't reach destination chain execution true, }, { "success: source callback", - fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract), types.CallbackTypeTimeoutPacket, true, }, { "success: dest callback with low gas (panic)", - fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, callbackAddr), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogPanicContract), + "none", // timeouts don't reach destination chain execution + true, + }, + { + "success: dest callback with low gas (error)", + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogErrorContract), "none", // timeouts don't reach destination chain execution true, }, { "failure: source callback with low gas (panic)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, callbackAddr), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract), + types.CallbackTypeSendPacket, + false, + }, + { + "failure: source callback with low gas (error)", + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, },