From 8123fdb6e76a24de6856c576da6ec8352908330c Mon Sep 17 00:00:00 2001 From: srdtrk Date: Thu, 17 Aug 2023 14:51:20 +0300 Subject: [PATCH 01/10] imp(callbacks/mock/contract_keeeper): improved contract keeper logic --- .../testing/simapp/contract_keeper.go | 67 +++++++++++++------ 1 file changed, 47 insertions(+), 20 deletions(-) diff --git a/modules/apps/callbacks/testing/simapp/contract_keeper.go b/modules/apps/callbacks/testing/simapp/contract_keeper.go index 0fc16f7a3bf..cfb8cdd1cf1 100644 --- a/modules/apps/callbacks/testing/simapp/contract_keeper.go +++ b/modules/apps/callbacks/testing/simapp/contract_keeper.go @@ -18,7 +18,19 @@ var _ callbacktypes.ContractKeeper = (*ContractKeeper)(nil) var ( StatefulCounterKey = "stateful-callback-counter" - MockCallbackUnauthorizedAddress = "cosmos15ulrf36d4wdtrtqzkgaan9ylwuhs7k7qz753uk" +) + +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. @@ -82,7 +94,7 @@ 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 @@ -97,7 +109,7 @@ 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 @@ -111,7 +123,7 @@ 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 @@ -124,17 +136,23 @@ func (k ContractKeeper) IBCReceivePacketCallback( 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. +// 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 // This function errors if the authAddress is MockCallbackUnauthorizedAddress. 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 +163,25 @@ 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)) - return ibcmock.MockApplicationCallbackError + switch contractAddress { + case ErrorContract: + 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: + ctx.GasMeter().ConsumeGas(gasRemaining/2, fmt.Sprintf("mock %s callback panic", callbackType)) + panic(ibcmock.MockApplicationCallbackError) + default: + 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 } From 91b7858ad70c983aa1f3dfd713d0e0a43415a9ca Mon Sep 17 00:00:00 2001 From: srdtrk Date: Thu, 17 Aug 2023 14:51:45 +0300 Subject: [PATCH 02/10] imp(callbacks_test): fixed transfer_test.go --- modules/apps/callbacks/transfer_test.go | 49 +++++++++++++++++++------ 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/modules/apps/callbacks/transfer_test.go b/modules/apps/callbacks/transfer_test.go index da523043f6b..c97626c5ecc 100644 --- a/modules/apps/callbacks/transfer_test.go +++ b/modules/apps/callbacks/transfer_test.go @@ -5,6 +5,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" transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" @@ -28,19 +29,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 +53,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 +77,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", "gas_limit": "450000"}}`, 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", "gas_limit": "450000"}}`, simapp.OogPanicContract), + types.CallbackTypeSendPacket, + false, + }, + { + "failure: dest callback with low gas (error)", + fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogErrorContract), + types.CallbackTypeReceivePacket, + false, + }, + { + "failure: source callback with low gas (error)", + fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, @@ -111,25 +124,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", "gas_limit": "450000"}}`, simapp.OogPanicContract), + "none", // timeouts don't reach destination chain execution + true, + }, + { + "success: dest callback with low gas (error)", + fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, 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", "gas_limit": "450000"}}`, simapp.OogPanicContract), + types.CallbackTypeSendPacket, + false, + }, + { + "failure: source callback with low gas (error)", + fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, From 17d1892b1709abc849ec2c1141d541c253be5819 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Thu, 17 Aug 2023 14:55:09 +0300 Subject: [PATCH 03/10] imp(callbacks_test): fixed ica_test.go --- modules/apps/callbacks/ica_test.go | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/modules/apps/callbacks/ica_test.go b/modules/apps/callbacks/ica_test.go index 600f278fccf..eabb706a6dc 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", "gas_limit": "350000"}}`, simapp.OogPanicContract), + types.CallbackTypeSendPacket, + false, + }, + { + "failure: source callback with low gas (error)", + fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "350000"}}`, 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", "gas_limit": "350000"}}`, simapp.OogPanicContract), + types.CallbackTypeSendPacket, + false, + }, + { + "failure: source callback with low gas (error)", + fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "350000"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, From caf674560f4326333116fd41f261645170bb6590 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Thu, 17 Aug 2023 15:00:06 +0300 Subject: [PATCH 04/10] imp(callbacks_test): fixed fee_transfer_test.go --- modules/apps/callbacks/fee_transfer_test.go | 49 ++++++++++++++++----- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/modules/apps/callbacks/fee_transfer_test.go b/modules/apps/callbacks/fee_transfer_test.go index f50c1d76abd..9f55e1082a8 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", "gas_limit": "450000"}}`, simapp.OogPanicContract), + types.CallbackTypeReceivePacket, + false, + }, + { + "failure: dest callback with low gas (error)", + fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, 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", "gas_limit": "450000"}}`, simapp.OogPanicContract), + types.CallbackTypeSendPacket, + false, + }, + { + "failure: source callback with low gas (error)", + fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, 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", "gas_limit": "450000"}}`, 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", "gas_limit": "450000"}}`, simapp.OogPanicContract), + types.CallbackTypeSendPacket, + false, + }, + { + "success: dest callback with low gas (error)", + fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogErrorContract), + "none", // timeouts don't reach destination chain execution + false, + }, + { + "failure: source callback with low gas (error)", + fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, From 25b461270ed36a9165e34ce14f4592ce53473625 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Thu, 17 Aug 2023 15:20:19 +0300 Subject: [PATCH 05/10] style(callbacks_test): removed unneeded gas_limit --- modules/apps/callbacks/fee_transfer_test.go | 16 ++++++++-------- modules/apps/callbacks/ica_test.go | 8 ++++---- modules/apps/callbacks/transfer_test.go | 16 ++++++++-------- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/modules/apps/callbacks/fee_transfer_test.go b/modules/apps/callbacks/fee_transfer_test.go index 9f55e1082a8..2d66f70bfa8 100644 --- a/modules/apps/callbacks/fee_transfer_test.go +++ b/modules/apps/callbacks/fee_transfer_test.go @@ -82,25 +82,25 @@ func (s *CallbacksTestSuite) TestIncentivizedTransferCallbacks() { }, { "failure: dest callback with low gas (panic)", - fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogPanicContract), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogPanicContract), types.CallbackTypeReceivePacket, false, }, { "failure: dest callback with low gas (error)", - fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogErrorContract), + 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"}}`, simapp.OogPanicContract), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract), types.CallbackTypeSendPacket, false, }, { "failure: source callback with low gas (error)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogErrorContract), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, @@ -152,25 +152,25 @@ func (s *CallbacksTestSuite) TestIncentivizedTransferTimeoutCallbacks() { }, { "success: dest callback with low gas (panic)", - fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogPanicContract), + 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"}}`, simapp.OogPanicContract), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract), types.CallbackTypeSendPacket, false, }, { "success: dest callback with low gas (error)", - fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogErrorContract), + 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", "gas_limit": "450000"}}`, simapp.OogErrorContract), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, diff --git a/modules/apps/callbacks/ica_test.go b/modules/apps/callbacks/ica_test.go index eabb706a6dc..1985477780d 100644 --- a/modules/apps/callbacks/ica_test.go +++ b/modules/apps/callbacks/ica_test.go @@ -65,13 +65,13 @@ func (s *CallbacksTestSuite) TestICACallbacks() { }, { "failure: source callback with low gas (panic)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "350000"}}`, simapp.OogPanicContract), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract), types.CallbackTypeSendPacket, false, }, { "failure: source callback with low gas (error)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "350000"}}`, simapp.OogErrorContract), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, @@ -115,13 +115,13 @@ func (s *CallbacksTestSuite) TestICATimeoutCallbacks() { }, { "failure: source callback with low gas (panic)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "350000"}}`, simapp.OogPanicContract), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract), types.CallbackTypeSendPacket, false, }, { "failure: source callback with low gas (error)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "350000"}}`, simapp.OogErrorContract), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, diff --git a/modules/apps/callbacks/transfer_test.go b/modules/apps/callbacks/transfer_test.go index c97626c5ecc..8802f318163 100644 --- a/modules/apps/callbacks/transfer_test.go +++ b/modules/apps/callbacks/transfer_test.go @@ -77,25 +77,25 @@ func (s *CallbacksTestSuite) TestTransferCallbacks() { }, { "failure: dest callback with low gas (panic)", - fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogPanicContract), + 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"}}`, simapp.OogPanicContract), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract), types.CallbackTypeSendPacket, false, }, { "failure: dest callback with low gas (error)", - fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogErrorContract), + fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeReceivePacket, false, }, { "failure: source callback with low gas (error)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogErrorContract), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, @@ -136,25 +136,25 @@ func (s *CallbacksTestSuite) TestTransferTimeoutCallbacks() { }, { "success: dest callback with low gas (panic)", - fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogPanicContract), + 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", "gas_limit": "450000"}}`, simapp.OogErrorContract), + 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"}}`, simapp.OogPanicContract), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract), types.CallbackTypeSendPacket, false, }, { "failure: source callback with low gas (error)", - fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, simapp.OogErrorContract), + fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract), types.CallbackTypeSendPacket, false, }, From 1ea7a3d0e035ce011b73b4d1d41d697aa7c4bd73 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Thu, 17 Aug 2023 16:43:28 +0300 Subject: [PATCH 06/10] style: ran golangci-lint --- .../testing/simapp/contract_keeper.go | 55 +++++++++---------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/modules/apps/callbacks/testing/simapp/contract_keeper.go b/modules/apps/callbacks/testing/simapp/contract_keeper.go index cfb8cdd1cf1..fb575dca5d0 100644 --- a/modules/apps/callbacks/testing/simapp/contract_keeper.go +++ b/modules/apps/callbacks/testing/simapp/contract_keeper.go @@ -16,21 +16,19 @@ import ( // MockKeeper implements callbacktypes.ContractKeeper var _ callbacktypes.ContractKeeper = (*ContractKeeper)(nil) -var ( - StatefulCounterKey = "stateful-callback-counter" -) +var StatefulCounterKey = "stateful-callback-counter" const ( // OogPanicContract is a contract address that will panic out of gas - OogPanicContract = "panics out of gas" + OogPanicContract = "panics out of gas" // OogErrorContract is a contract address that will error out of gas - OogErrorContract = "errors out of gas" + OogErrorContract = "errors out of gas" // PanicContract is a contract address that will panic - PanicContract = "panics" + PanicContract = "panics" // ErrorContract is a contract address that will return an error - ErrorContract = "errors" + ErrorContract = "errors" // SuccessContract is a contract address that will return nil - SuccessContract = "success" + SuccessContract = "success" ) // This is a mock contract keeper used for testing. It is not wired up to any modules. @@ -142,11 +140,12 @@ func (k ContractKeeper) IBCReceivePacketCallback( // 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 +// - 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 +// // This function errors if the authAddress is MockCallbackUnauthorizedAddress. func (k ContractKeeper) processMockCallback( ctx sdk.Context, @@ -164,24 +163,24 @@ func (k ContractKeeper) processMockCallback( k.Counters[callbackType]++ switch contractAddress { - case ErrorContract: - 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: - ctx.GasMeter().ConsumeGas(gasRemaining/2, fmt.Sprintf("mock %s callback panic", callbackType)) - panic(ibcmock.MockApplicationCallbackError) - default: - ctx.GasMeter().ConsumeGas(gasRemaining/2, fmt.Sprintf("mock %s callback success", callbackType)) - return nil // success + case ErrorContract: + 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: + ctx.GasMeter().ConsumeGas(gasRemaining/2, fmt.Sprintf("mock %s callback panic", callbackType)) + panic(ibcmock.MockApplicationCallbackError) + default: + ctx.GasMeter().ConsumeGas(gasRemaining/2, fmt.Sprintf("mock %s callback success", callbackType)) + return nil // success } } From 75bb8fa209758742a46eef0f889f1c95408eceb9 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Thu, 17 Aug 2023 16:44:00 +0300 Subject: [PATCH 07/10] imp(callbacks_test): implemented simplified logic --- modules/apps/callbacks/ibc_middleware_test.go | 91 ++++++++++--------- 1 file changed, 49 insertions(+), 42 deletions(-) diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go index 83383eee239..ff7d92069a7 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)}, }, } @@ -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, @@ -272,7 +276,7 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() { 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), @@ -356,13 +360,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 +393,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 +402,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 +462,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 +509,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 +551,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,20 +560,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, }, - /* - TODO: https://github.com/cosmos/ibc-go/issues/4309 - { - "failure: callback execution fails", - func() {}, - callbackFailed, - successAck, + { + "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 { @@ -578,7 +585,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), From 332f08a1aa6c8e98959ab0826b48ca01afa5fbf0 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Thu, 17 Aug 2023 16:46:01 +0300 Subject: [PATCH 08/10] imp(callbacks_test): removed unneeded variable --- modules/apps/callbacks/ibc_middleware_test.go | 8 ++++---- modules/apps/callbacks/transfer_test.go | 2 -- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go index ff7d92069a7..1073d9e8ba7 100644 --- a/modules/apps/callbacks/ibc_middleware_test.go +++ b/modules/apps/callbacks/ibc_middleware_test.go @@ -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) @@ -278,8 +278,8 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() { // set user gas limit above panic level in mock contract keeper 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{ diff --git a/modules/apps/callbacks/transfer_test.go b/modules/apps/callbacks/transfer_test.go index 8802f318163..c2f3e32fbde 100644 --- a/modules/apps/callbacks/transfer_test.go +++ b/modules/apps/callbacks/transfer_test.go @@ -12,8 +12,6 @@ import ( ibctesting "github.com/cosmos/ibc-go/v7/testing" ) -var callbackAddr = ibctesting.TestAccAddress - func (s *CallbacksTestSuite) TestTransferCallbacks() { testCases := []struct { name string From e14aba1e3f2e146e51bdfa8a5f649a82ac22597a Mon Sep 17 00:00:00 2001 From: srdtrk Date: Wed, 23 Aug 2023 17:35:02 +0300 Subject: [PATCH 09/10] imp(callbacks): implemented some review feedback --- modules/apps/callbacks/ibc_middleware_test.go | 1 - modules/apps/callbacks/testing/simapp/contract_keeper.go | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go index f61c26a1f33..dd389133782 100644 --- a/modules/apps/callbacks/ibc_middleware_test.go +++ b/modules/apps/callbacks/ibc_middleware_test.go @@ -275,7 +275,6 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() { s.Run(tc.name, func() { s.SetupTransferTest() - // set user gas limit above panic level in mock contract keeper userGasLimit = 600000 packetData = transfertypes.NewFungibleTokenPacketData( ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), ibctesting.TestAccAddress, ibctesting.TestAccAddress, diff --git a/modules/apps/callbacks/testing/simapp/contract_keeper.go b/modules/apps/callbacks/testing/simapp/contract_keeper.go index fb575dca5d0..9629fac8925 100644 --- a/modules/apps/callbacks/testing/simapp/contract_keeper.go +++ b/modules/apps/callbacks/testing/simapp/contract_keeper.go @@ -164,6 +164,7 @@ func (k ContractKeeper) processMockCallback( 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: @@ -177,9 +178,11 @@ func (k ContractKeeper) processMockCallback( 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 } From 95b25e5e201641069c3eafc2b2a2530121b8b027 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Thu, 24 Aug 2023 01:20:41 +0300 Subject: [PATCH 10/10] docs(callbacks/simapp): updated godocs of mock contract keeper functions --- .../testing/simapp/contract_keeper.go | 46 +++++++++++-------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/modules/apps/callbacks/testing/simapp/contract_keeper.go b/modules/apps/callbacks/testing/simapp/contract_keeper.go index 9629fac8925..1e0d9e702f2 100644 --- a/modules/apps/callbacks/testing/simapp/contract_keeper.go +++ b/modules/apps/callbacks/testing/simapp/contract_keeper.go @@ -78,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, @@ -95,10 +98,13 @@ func (k ContractKeeper) IBCSendPacketCallback( 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, @@ -110,10 +116,13 @@ func (k ContractKeeper) IBCOnAcknowledgementPacketCallback( 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, @@ -124,10 +133,13 @@ func (k ContractKeeper) IBCOnTimeoutPacketCallback( 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, @@ -145,8 +157,6 @@ func (k ContractKeeper) IBCReceivePacketCallback( // - 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 -// -// This function errors if the authAddress is MockCallbackUnauthorizedAddress. func (k ContractKeeper) processMockCallback( ctx sdk.Context, callbackType callbacktypes.CallbackType,