From 96090a74c4291fa410a413fc7c2ba39fef18e621 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Mon, 14 Aug 2023 11:58:47 +0000 Subject: [PATCH] Remove callbacks wiring in ibc go simapp (#4340) --- modules/apps/callbacks/ibc_middleware_test.go | 15 +- testing/mock/contract_keeper.go | 154 ------------------ testing/mock/mock.go | 3 - testing/simapp/app.go | 28 +--- 4 files changed, 13 insertions(+), 187 deletions(-) delete mode 100644 testing/mock/contract_keeper.go diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go index dfee31d0518..e7a2dadc747 100644 --- a/modules/apps/callbacks/ibc_middleware_test.go +++ b/modules/apps/callbacks/ibc_middleware_test.go @@ -9,6 +9,7 @@ import ( icacontrollertypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types" ibccallbacks "github.com/cosmos/ibc-go/v7/modules/apps/callbacks" + "github.com/cosmos/ibc-go/v7/modules/apps/callbacks/testing/simapp" "github.com/cosmos/ibc-go/v7/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" @@ -30,14 +31,14 @@ func (s *CallbacksTestSuite) TestNewIBCMiddleware() { { "success", func() { - _ = ibccallbacks.NewIBCMiddleware(ibcmock.IBCModule{}, channelkeeper.Keeper{}, ibcmock.ContractKeeper{}, maxCallbackGas) + _ = ibccallbacks.NewIBCMiddleware(ibcmock.IBCModule{}, channelkeeper.Keeper{}, simapp.ContractKeeper{}, maxCallbackGas) }, nil, }, { "panics with nil underlying app", func() { - _ = ibccallbacks.NewIBCMiddleware(nil, channelkeeper.Keeper{}, ibcmock.ContractKeeper{}, maxCallbackGas) + _ = ibccallbacks.NewIBCMiddleware(nil, channelkeeper.Keeper{}, simapp.ContractKeeper{}, maxCallbackGas) }, fmt.Errorf("underlying application does not implement %T", (*types.CallbacksCompatibleModule)(nil)), }, @@ -51,14 +52,14 @@ func (s *CallbacksTestSuite) TestNewIBCMiddleware() { { "panics with nil ics4Wrapper", func() { - _ = ibccallbacks.NewIBCMiddleware(ibcmock.IBCModule{}, nil, ibcmock.ContractKeeper{}, maxCallbackGas) + _ = ibccallbacks.NewIBCMiddleware(ibcmock.IBCModule{}, nil, simapp.ContractKeeper{}, maxCallbackGas) }, fmt.Errorf("ICS4Wrapper cannot be nil"), }, { "panics with zero maxCallbackGas", func() { - _ = ibccallbacks.NewIBCMiddleware(ibcmock.IBCModule{}, channelkeeper.Keeper{}, ibcmock.ContractKeeper{}, uint64(0)) + _ = ibccallbacks.NewIBCMiddleware(ibcmock.IBCModule{}, channelkeeper.Keeper{}, simapp.ContractKeeper{}, uint64(0)) }, fmt.Errorf("maxCallbackGas cannot be zero"), }, @@ -128,7 +129,7 @@ func (s *CallbacksTestSuite) TestSendPacket() { { "failure: callback execution fails, sender is not callback address", func() { - packetData.Sender = ibcmock.MockCallbackUnauthorizedAddress + packetData.Sender = simapp.MockCallbackUnauthorizedAddress }, types.CallbackTypeSendPacket, false, @@ -257,7 +258,7 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() { { "failure: callback execution fails, unauthorized address", func() { - packetData.Sender = ibcmock.MockCallbackUnauthorizedAddress + packetData.Sender = simapp.MockCallbackUnauthorizedAddress packet.Data = packetData.GetBytes() }, callbackFailed, @@ -407,7 +408,7 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() { { "failure: callback execution fails, unauthorized address", func() { - packetData.Sender = ibcmock.MockCallbackUnauthorizedAddress + packetData.Sender = simapp.MockCallbackUnauthorizedAddress packet.Data = packetData.GetBytes() }, callbackFailed, diff --git a/testing/mock/contract_keeper.go b/testing/mock/contract_keeper.go deleted file mode 100644 index d030de2393b..00000000000 --- a/testing/mock/contract_keeper.go +++ /dev/null @@ -1,154 +0,0 @@ -package mock - -import ( - "fmt" - - storetypes "github.com/cosmos/cosmos-sdk/store/types" - sdk "github.com/cosmos/cosmos-sdk/types" - - callbacktypes "github.com/cosmos/ibc-go/v7/modules/apps/callbacks/types" - clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" - channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" - ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported" -) - -// MockKeeper implements callbacktypes.ContractKeeper -var _ callbacktypes.ContractKeeper = (*ContractKeeper)(nil) - -// This is a mock contract keeper used for testing. It is not wired up to any modules. -// It implements the interface functions expected by the ibccallbacks middleware -// so that it can be tested with simapp. The keeper is responsible for tracking -// two metrics: -// - number of callbacks called per callback type -// - stateful entry attempts -// -// The counter for callbacks allows us to ensure the correct callbacks were routed to -// and the stateful entries allows us to track state reversals or reverted state upon -// contract execution failure or out of gas errors. -type ContractKeeper struct { - key storetypes.StoreKey - - Counters map[callbacktypes.CallbackType]int -} - -// SetStateEntryCounter sets state entry counter. The number of stateful -// entries is tracked as a uint8. This function is used to test state reversals. -func (k ContractKeeper) SetStateEntryCounter(ctx sdk.Context, count uint8) { - store := ctx.KVStore(k.key) - store.Set([]byte(StatefulCounterKey), []byte{count}) -} - -// GetStateEntryCounter returns the state entry counter stored in state. -func (k ContractKeeper) GetStateEntryCounter(ctx sdk.Context) uint8 { - store := ctx.KVStore(k.key) - bz := store.Get([]byte(StatefulCounterKey)) - if bz == nil { - return 0 - } - return bz[0] -} - -// IncrementStatefulCounter increments the stateful callback counter in state. -func (k ContractKeeper) IncrementStateEntryCounter(ctx sdk.Context) { - count := k.GetStateEntryCounter(ctx) - k.SetStateEntryCounter(ctx, count+1) -} - -// NewKeeper creates a new mock ContractKeeper. -func NewContractKeeper(key storetypes.StoreKey) ContractKeeper { - return ContractKeeper{ - key: key, - Counters: make(map[callbacktypes.CallbackType]int), - } -} - -// 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. -func (k ContractKeeper) IBCSendPacketCallback( - ctx sdk.Context, - sourcePort string, - sourceChannel string, - timeoutHeight clienttypes.Height, - timeoutTimestamp uint64, - packetData []byte, - contractAddress, - packetSenderAddress string, -) error { - return k.processMockCallback(ctx, callbacktypes.CallbackTypeSendPacket, packetSenderAddress) -} - -// 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. -func (k ContractKeeper) IBCOnAcknowledgementPacketCallback( - ctx sdk.Context, - packet channeltypes.Packet, - acknowledgement []byte, - relayer sdk.AccAddress, - contractAddress, - packetSenderAddress string, -) error { - return k.processMockCallback(ctx, callbacktypes.CallbackTypeAcknowledgementPacket, packetSenderAddress) -} - -// 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. -func (k ContractKeeper) IBCOnTimeoutPacketCallback( - ctx sdk.Context, - packet channeltypes.Packet, - relayer sdk.AccAddress, - contractAddress, - packetSenderAddress string, -) error { - return k.processMockCallback(ctx, callbacktypes.CallbackTypeTimeoutPacket, packetSenderAddress) -} - -// 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. -func (k ContractKeeper) IBCReceivePacketCallback( - ctx sdk.Context, - packet ibcexported.PacketI, - ack ibcexported.Acknowledgement, - contractAddress string, -) error { - return k.processMockCallback(ctx, callbacktypes.CallbackTypeReceivePacket, "") -} - -// 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. -func (k ContractKeeper) processMockCallback( - ctx sdk.Context, - callbackType callbacktypes.CallbackType, - authAddress string, -) error { - gasRemaining := ctx.GasMeter().GasRemaining() - - // increment stateful entries, if the callbacks module handler - // reverts state, we can check by querying for the counter - // currently stored. - k.IncrementStateEntryCounter(ctx) - - // 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 MockApplicationCallbackError - } - - ctx.GasMeter().ConsumeGas(500000, fmt.Sprintf("mock %s callback success", callbackType)) - return nil -} diff --git a/testing/mock/mock.go b/testing/mock/mock.go index 9913e6ead70..88c46c1bfb7 100644 --- a/testing/mock/mock.go +++ b/testing/mock/mock.go @@ -33,8 +33,6 @@ const ( ) var ( - StatefulCounterKey = "stateful-callback-counter" - MockAcknowledgement = channeltypes.NewResultAcknowledgement([]byte("mock acknowledgement")) MockFailAcknowledgement = channeltypes.NewErrorAcknowledgement(fmt.Errorf("mock failed acknowledgement")) MockPacketData = []byte("mock packet data") @@ -43,7 +41,6 @@ var ( MockRecvCanaryCapabilityName = "mock receive canary capability name" MockAckCanaryCapabilityName = "mock acknowledgement canary capability name" MockTimeoutCanaryCapabilityName = "mock timeout canary capability name" - MockCallbackUnauthorizedAddress = "cosmos15ulrf36d4wdtrtqzkgaan9ylwuhs7k7qz753uk" // MockApplicationCallbackError should be returned when an application callback should fail. It is possible to // test that this error was returned using ErrorIs. MockApplicationCallbackError error = &applicationCallbackError{} diff --git a/testing/simapp/app.go b/testing/simapp/app.go index 9567d04018b..b267158af08 100644 --- a/testing/simapp/app.go +++ b/testing/simapp/app.go @@ -112,7 +112,6 @@ import ( ibcfee "github.com/cosmos/ibc-go/v7/modules/apps/29-fee" ibcfeekeeper "github.com/cosmos/ibc-go/v7/modules/apps/29-fee/keeper" ibcfeetypes "github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types" - ibccallbacks "github.com/cosmos/ibc-go/v7/modules/apps/callbacks" transfer "github.com/cosmos/ibc-go/v7/modules/apps/transfer" ibctransferkeeper "github.com/cosmos/ibc-go/v7/modules/apps/transfer/keeper" ibctransfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types" @@ -248,9 +247,6 @@ type SimApp struct { ScopedIBCMockKeeper capabilitykeeper.ScopedKeeper ScopedICAMockKeeper capabilitykeeper.ScopedKeeper - // mock contract keeper used for testing - MockContractKeeper ibcmock.ContractKeeper - // make IBC modules public for test purposes // these modules are never directly routed to by the IBC Router ICAAuthModule ibcmock.IBCModule @@ -439,10 +435,6 @@ func NewSimApp( appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), app.StakingKeeper, app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), ) - // NOTE: The mock ContractKeeper is only created for testing. - // Real applications should not use the mock ContractKeeper - app.MockContractKeeper = ibcmock.NewContractKeeper(memKeys[ibcmock.MemStoreKey]) - // register the proposal types govRouter := govv1beta1.NewRouter() govRouter.AddRoute(govtypes.RouterKey, govv1beta1.ProposalHandler). @@ -499,11 +491,9 @@ func NewSimApp( ibcRouter := porttypes.NewRouter() // Middleware Stacks - maxCallbackGas := uint64(1_000_000) // Create Transfer Keeper and pass IBCFeeKeeper as expected Channel and PortKeeper // since fee middleware will wrap the IBCKeeper for underlying application. - // NOTE: the Transfer Keeper's ICS4Wrapper can later be replaced. app.TransferKeeper = ibctransferkeeper.NewKeeper( appCodec, keys[ibctransfertypes.StoreKey], app.GetSubspace(ibctransfertypes.ModuleName), app.IBCFeeKeeper, // ISC4 Wrapper: fee IBC middleware @@ -525,13 +515,12 @@ func NewSimApp( // Create Transfer Stack // SendPacket, since it is originating from the application to core IBC: - // transferKeeper.SendPacket -> fee.SendPacket -> callbacks.SendPacket -> channel.SendPacket + // transferKeeper.SendPacket -> fee.SendPacket -> channel.SendPacket // RecvPacket, message that originates from core IBC and goes down to app, the flow is the other way - // channel.RecvPacket -> callbacks.OnRecvPacket -> fee.OnRecvPacket -> transfer.OnRecvPacket + // channel.RecvPacket -> fee.OnRecvPacket -> transfer.OnRecvPacket // transfer stack contains (from top to bottom): - // - IBC Callbacks Middleware // - IBC Fee Middleware // - Transfer @@ -539,16 +528,13 @@ func NewSimApp( var transferStack porttypes.IBCModule transferStack = transfer.NewIBCModule(app.TransferKeeper) transferStack = ibcfee.NewIBCMiddleware(transferStack, app.IBCFeeKeeper) - transferStack = ibccallbacks.NewIBCMiddleware(transferStack, app.IBCFeeKeeper, app.MockContractKeeper, maxCallbackGas) - // Since the callbacks middleware itself is an ics4wrapper, it needs to be passed to the transfer keeper - app.TransferKeeper.WithICS4Wrapper(transferStack.(porttypes.Middleware)) // Add transfer stack to IBC Router ibcRouter.AddRoute(ibctransfertypes.ModuleName, transferStack) // Create Interchain Accounts Stack // SendPacket, since it is originating from the application to core IBC: - // icaAuthModuleKeeper.SendTx -> icaController.SendPacket -> fee.SendPacket -> callbacks.SendPacket -> channel.SendPacket + // icaAuthModuleKeeper.SendTx -> icaController.SendPacket -> fee.SendPacket -> channel.SendPacket // initialize ICA module with mock module as the authentication module on the controller side var icaControllerStack porttypes.IBCModule @@ -556,12 +542,9 @@ func NewSimApp( app.ICAAuthModule = icaControllerStack.(ibcmock.IBCModule) icaControllerStack = icacontroller.NewIBCMiddleware(icaControllerStack, app.ICAControllerKeeper) icaControllerStack = ibcfee.NewIBCMiddleware(icaControllerStack, app.IBCFeeKeeper) - icaControllerStack = ibccallbacks.NewIBCMiddleware(icaControllerStack, app.IBCFeeKeeper, app.MockContractKeeper, maxCallbackGas) - // Since the callbacks middleware itself is an ics4wrapper, it needs to be passed to the ica controller keeper - app.ICAControllerKeeper.WithICS4Wrapper(icaControllerStack.(porttypes.Middleware)) // RecvPacket, message that originates from core IBC and goes down to app, the flow is: - // channel.RecvPacket -> callbacks.OnRecvPacket -> fee.OnRecvPacket -> icaHost.OnRecvPacket + // channel.RecvPacket -> fee.OnRecvPacket -> icaHost.OnRecvPacket var icaHostStack porttypes.IBCModule icaHostStack = icahost.NewIBCModule(app.ICAHostKeeper) @@ -589,8 +572,7 @@ func NewSimApp( // create fee wrapped mock module feeMockModule := ibcmock.NewIBCModule(&mockModule, ibcmock.NewIBCApp(MockFeePort, scopedFeeMockKeeper)) app.FeeMockModule = feeMockModule - var feeWithMockModule porttypes.Middleware = ibcfee.NewIBCMiddleware(feeMockModule, app.IBCFeeKeeper) - feeWithMockModule = ibccallbacks.NewIBCMiddleware(feeWithMockModule, app.IBCFeeKeeper, app.MockContractKeeper, maxCallbackGas) + feeWithMockModule := ibcfee.NewIBCMiddleware(feeMockModule, app.IBCFeeKeeper) ibcRouter.AddRoute(MockFeePort, feeWithMockModule) // Seal the IBC Router