Apply review feedback (CosmWasm#1376)

* Apply review feedback

* Better test name

* Add contract integration test

* Better doc in test and minor refactoring

x/wasm/ibc.go

@@ -273,16 +273,13 @@ func (i IBCHandler) OnRecvPacket(
 	ack, err := i.keeper.OnRecvPacket(ctx.WithEventManager(em), contractAddr, msg)
 	if err != nil {
 		ack = channeltypes.NewErrorAcknowledgement(err)
-		types.EmitAcknowledgementEvent(ctx, contractAddr, ack, err)
 		// the state gets reverted, so we drop all captured events
-		return ack
-	}
-	if ack == nil || ack.Success() {
+	} else if ack == nil || ack.Success() {
 		// emit all contract and submessage events on success
 		// nil ack is a success case, see: https://github.com/cosmos/ibc-go/blob/v7.0.0/modules/core/keeper/msg_server.go#L453
 		ctx.EventManager().EmitEvents(em.Events())
 	}
-	types.EmitAcknowledgementEvent(ctx, contractAddr, ack, nil)
+	types.EmitAcknowledgementEvent(ctx, contractAddr, ack, err)
 	return ack
 }
 

x/wasm/ibc_integration_test.go

@@ -3,6 +3,10 @@ package wasm_test
 import (
 	"testing"
 
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
+	"github.com/CosmWasm/wasmd/x/wasm/types"
+
 	wasmvm "github.com/CosmWasm/wasmvm"
 	wasmvmtypes "github.com/CosmWasm/wasmvm/types"
 	ibctransfertypes "github.com/cosmos/ibc-go/v4/modules/apps/transfer/types"
@@ -124,3 +128,141 @@ func TestOnChanOpenTryVersion(t *testing.T) {
 		})
 	}
 }
+
+func TestOnIBCPacketReceive(t *testing.T) {
+	// given 2 chains with a mock on chain A to control the IBC flow
+	// and  the ibc-reflect contract on chain B
+	// when the test package is relayed
+	// then the contract executes the flow defined for the packet data
+	// and  the ibc Ack captured is what we expect
+	specs := map[string]struct {
+		packetData          []byte
+		expAck              []byte
+		expPacketNotHandled bool
+	}{
+		"all good": {
+			packetData: []byte(`{"who_am_i":{}}`),
+			expAck:     []byte(`{"ok":{"account":"cosmos1suhgf5svhu4usrurvxzlgn54ksxmn8gljarjtxqnapv8kjnp4nrs2zhgh2"}}`),
+		},
+		"with result err": {
+			packetData: []byte(`{"return_err": {"text": "my error"}}`),
+			expAck:     []byte(`{"error":"invalid packet: Generic error: my error"}`),
+		},
+		"with returned msg fails": {
+			packetData: []byte(`{"return_msgs": {"msgs": [{"bank":{"send":{"to_address": "invalid-address", "amount": [{"denom": "ALX", "amount": "1"}]}}}]}}`),
+			expAck:     []byte(`{"error":"ABCI code: 7: error handling packet: see events for details"}`),
+		},
+		"with contract panic": {
+			packetData:          []byte(`{"panic":{}}`),
+			expPacketNotHandled: true,
+		},
+	}
+	for name, spec := range specs {
+		t.Run(name, func(t *testing.T) {
+			mockContractEngine := NewCaptureAckTestContractEngine()
+			chainAOpts := []wasmkeeper.Option{
+				wasmkeeper.WithWasmEngine(mockContractEngine),
+			}
+			var (
+				coord  = wasmibctesting.NewCoordinator(t, 2, chainAOpts)
+				chainA = coord.GetChain(wasmibctesting.GetChainID(0))
+				chainB = coord.GetChain(wasmibctesting.GetChainID(1))
+			)
+			// setup chain A contract metadata for mock
+			myMockContractAddr := chainA.SeedNewContractInstance() // setups env but uses mock contract
+
+			// setup chain B contracts
+			reflectID := chainB.StoreCodeFile("./keeper/testdata/reflect.wasm").CodeID
+			initMsg := wasmkeeper.IBCReflectInitMsg{ReflectCodeID: reflectID}.GetBytes(t)
+			codeID := chainB.StoreCodeFile("./keeper/testdata/ibc_reflect.wasm").CodeID
+			ibcReflectContractAddr := chainB.InstantiateContract(codeID, initMsg)
+
+			// establish IBC channels
+			var (
+				sourcePortID      = chainA.ContractInfo(myMockContractAddr).IBCPortID
+				counterpartPortID = chainB.ContractInfo(ibcReflectContractAddr).IBCPortID
+				path              = wasmibctesting.NewPath(chainA, chainB)
+			)
+			path.EndpointA.ChannelConfig = &ibctesting.ChannelConfig{
+				PortID: sourcePortID, Version: "ibc-reflect-v1", Order: channeltypes.ORDERED,
+			}
+			path.EndpointB.ChannelConfig = &ibctesting.ChannelConfig{
+				PortID: counterpartPortID, Version: "ibc-reflect-v1", Order: channeltypes.ORDERED,
+			}
+
+			coord.SetupConnections(path)
+			coord.CreateChannels(path)
+			coord.CommitBlock(chainA, chainB)
+			require.Equal(t, 0, len(chainA.PendingSendPackets))
+			require.Equal(t, 0, len(chainB.PendingSendPackets))
+
+			// when an ibc packet is sent from chain A to chain B
+			capturedAck := mockContractEngine.SubmitIBCPacket(t, path, chainA, myMockContractAddr, spec.packetData)
+			coord.CommitBlock(chainA, chainB)
+
+			require.Equal(t, 1, len(chainA.PendingSendPackets))
+			require.Equal(t, 0, len(chainB.PendingSendPackets))
+
+			err := coord.RelayAndAckPendingPackets(path)
+
+			// then
+			if spec.expPacketNotHandled {
+				const contractPanicToErrMsg = `recovered: Error calling the VM: Error executing Wasm: Wasmer runtime error: RuntimeError: Aborted: panicked at 'This page intentionally faulted', src/contract.rs:316:5`
+				assert.ErrorContains(t, err, contractPanicToErrMsg)
+				require.Nil(t, *capturedAck)
+				return
+			}
+			require.NoError(t, err)
+			if spec.expAck != nil {
+				assert.Equal(t, spec.expAck, *capturedAck, string(*capturedAck))
+			} else {
+				require.Nil(t, *capturedAck)
+			}
+		})
+	}
+}
+
+// mock to submit an ibc data package from given chain and capture the ack
+type captureAckTestContractEngine struct {
+	*wasmtesting.MockWasmer
+}
+
+// NewCaptureAckTestContractEngine constructor
+func NewCaptureAckTestContractEngine() *captureAckTestContractEngine {
+	m := wasmtesting.NewIBCContractMockWasmer(&wasmtesting.MockIBCContractCallbacks{
+		IBCChannelOpenFn: func(codeID wasmvm.Checksum, env wasmvmtypes.Env, msg wasmvmtypes.IBCChannelOpenMsg, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.IBC3ChannelOpenResponse, uint64, error) {
+			return &wasmvmtypes.IBC3ChannelOpenResponse{}, 0, nil
+		},
+		IBCChannelConnectFn: func(codeID wasmvm.Checksum, env wasmvmtypes.Env, msg wasmvmtypes.IBCChannelConnectMsg, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.IBCBasicResponse, uint64, error) {
+			return &wasmvmtypes.IBCBasicResponse{}, 0, nil
+		},
+	})
+	return &captureAckTestContractEngine{m}
+}
+
+// SubmitIBCPacket starts an IBC packet transfer on given chain and captures the ack returned
+func (x *captureAckTestContractEngine) SubmitIBCPacket(t *testing.T, path *wasmibctesting.Path, chainA *wasmibctesting.TestChain, senderContractAddr sdk.AccAddress, packetData []byte) *[]byte {
+	// prepare a bridge to send an ibc packet by an ordinary wasm execute message
+	x.MockWasmer.ExecuteFn = func(codeID wasmvm.Checksum, env wasmvmtypes.Env, info wasmvmtypes.MessageInfo, executeMsg []byte, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.Response, uint64, error) {
+		return &wasmvmtypes.Response{
+			Messages: []wasmvmtypes.SubMsg{{ID: 1, ReplyOn: wasmvmtypes.ReplyNever, Msg: wasmvmtypes.CosmosMsg{IBC: &wasmvmtypes.IBCMsg{SendPacket: &wasmvmtypes.SendPacketMsg{
+				ChannelID: path.EndpointA.ChannelID, Data: executeMsg, Timeout: wasmvmtypes.IBCTimeout{Block: &wasmvmtypes.IBCTimeoutBlock{Revision: 1, Height: 10000000}},
+			}}}}},
+		}, 0, nil
+	}
+	// capture acknowledgement
+	var gotAck []byte
+	x.MockWasmer.IBCPacketAckFn = func(codeID wasmvm.Checksum, env wasmvmtypes.Env, msg wasmvmtypes.IBCPacketAckMsg, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.IBCBasicResponse, uint64, error) {
+		gotAck = msg.Acknowledgement.Data
+		return &wasmvmtypes.IBCBasicResponse{}, 0, nil
+	}
+
+	// start the process
+	_, err := chainA.SendMsgs(&types.MsgExecuteContract{
+		Sender:   chainA.SenderAccount.GetAddress().String(),
+		Contract: senderContractAddr.String(),
+		Msg:      packetData,
+	})
+	require.NoError(t, err)
+	return &gotAck
+}

x/wasm/ibc_test.go

@@ -25,12 +25,12 @@ func TestOnRecvPacket(t *testing.T) {
 	})
 	myCustomEvent := sdk.NewEvent("testing")
 	specs := map[string]struct {
-		ibcPkg      channeltypes.Packet
-		contractRsp ibcexported.Acknowledgement
-		contractErr error
-		expEvents   sdk.Events
-		expPanic    bool
-		expAck      ibcexported.Acknowledgement
+		ibcPkg               channeltypes.Packet
+		contractRsp          ibcexported.Acknowledgement
+		contractOkMsgExecErr error
+		expEvents            sdk.Events
+		expPanic             bool
+		expAck               ibcexported.Acknowledgement
 	}{
 		"contract returns success response": {
 			ibcPkg:      anyContractIBCPkg,
@@ -83,10 +83,10 @@ func TestOnRecvPacket(t *testing.T) {
 			}),
 			expPanic: true,
 		},
-		"contract executed with error": {
-			ibcPkg:      anyContractIBCPkg,
-			contractErr: types.ErrInvalid.Wrap("testing"),
-			expAck:      channeltypes.NewErrorAcknowledgement(types.ErrInvalid.Wrap("testing")),
+		"returned messages executed with error": {
+			ibcPkg:               anyContractIBCPkg,
+			contractOkMsgExecErr: types.ErrInvalid.Wrap("testing"),
+			expAck:               channeltypes.NewErrorAcknowledgement(types.ErrInvalid.Wrap("testing")),
 			expEvents: sdk.Events{{
 				Type: "ibc_packet_received",
 				Attributes: []abci.EventAttribute{
@@ -104,7 +104,7 @@ func TestOnRecvPacket(t *testing.T) {
 				OnRecvPacketFn: func(ctx sdk.Context, contractAddr sdk.AccAddress, msg wasmvmtypes.IBCPacketReceiveMsg) (ibcexported.Acknowledgement, error) {
 					// additional custom event to confirm event handling on state commit/ rollback
 					ctx.EventManager().EmitEvent(myCustomEvent)
-					return spec.contractRsp, spec.contractErr
+					return spec.contractRsp, spec.contractOkMsgExecErr
 				},
 			}
 			h := NewIBCHandler(mock, nil, nil)

x/wasm/keeper/relay.go

@@ -133,7 +133,10 @@ func (k Keeper) OnRecvPacket(
 	res, gasUsed, execErr := k.wasmVM.IBCPacketReceive(codeInfo.CodeHash, env, msg, prefixStore, cosmwasmAPI, querier, ctx.GasMeter(), gas, costJSONDeserialization)
 	k.consumeRuntimeGas(ctx, gasUsed)
 	if execErr != nil {
-		panic(execErr) // let contract fully abort IBC receive in certain case
+		panic(execErr) // let the contract fully abort an IBC packet receive.
+		// Throwing a panic here instead of an error ack will revert
+		// all state downstream and not persist any data in ibc-go.
+		// This can be triggered by throwing a panic in the contract
 	}
 	if res.Err != "" {
 		// return error ACK with non-redacted contract message, state will be reverted

x/wasm/keeper/relay_test.go

@@ -355,15 +355,8 @@ func TestOnRecvPacket(t *testing.T) {
 		"contract aborts tx with error": {
 			contractAddr:   example.Contract,
 			expContractGas: myContractGas,
-			contractResp: &wasmvmtypes.IBCReceiveResult{
-				Ok: &wasmvmtypes.IBCReceiveResponse{
-					Acknowledgement: []byte("myAck"),
-					Messages:        []wasmvmtypes.SubMsg{{ReplyOn: wasmvmtypes.ReplyNever, Msg: wasmvmtypes.CosmosMsg{Bank: &wasmvmtypes.BankMsg{}}}},
-					Attributes:      []wasmvmtypes.EventAttribute{{Key: "Foo", Value: "Bar"}},
-				},
-			},
-			contractErr: errors.New("test, ignore"),
-			expPanic:    true,
+			contractErr:    errors.New("test, ignore"),
+			expPanic:       true,
 		},
 		"dispatch contract messages on success": {
 			contractAddr:   example.Contract,