Merge pull request cosmos#589 from CosmWasm/577-reply-events-2

Filter events before reply block
notional-labs · Aug 12, 2021 · 1933578 · 1933578
2 parents 7b2e84c + 7fbf513
commit 1933578
Show file tree

Hide file tree

Showing 5 changed files with 92 additions and 41 deletions.
diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go
@@ -1072,19 +1072,12 @@ func NewDefaultWasmVMContractResponseHandler(md msgDispatcher) *DefaultWasmVMCon
 
 // Handle processes the data returned by a contract invocation.
 func (h DefaultWasmVMContractResponseHandler) Handle(ctx sdk.Context, contractAddr sdk.AccAddress, ibcPort string, messages []wasmvmtypes.SubMsg, origRspData []byte) ([]byte, error) {
-	em := sdk.NewEventManager()
 	result := origRspData
-	switch rsp, err := h.md.DispatchSubmessages(ctx.WithEventManager(em), contractAddr, ibcPort, messages); {
+	switch rsp, err := h.md.DispatchSubmessages(ctx, contractAddr, ibcPort, messages); {
 	case err != nil:
 		return nil, sdkerrors.Wrap(err, "submessages")
 	case rsp != nil:
 		result = rsp
 	}
-	// emit non message type events only
-	for _, e := range em.Events() {
-		if e.Type != sdk.EventTypeMessage {
-			ctx.EventManager().EmitEvent(e)
-		}
-	}
 	return result, nil
 }
diff --git a/x/wasm/keeper/keeper_test.go b/x/wasm/keeper/keeper_test.go
@@ -1563,15 +1563,6 @@ func TestNewDefaultWasmVMContractResponseHandler(t *testing.T) {
 			},
 			expErr: true,
 		},
-		"message events filtered out": {
-			setup: func(m *wasmtesting.MockMsgDispatcher) {
-				m.DispatchSubmessagesFn = func(ctx sdk.Context, contractAddr sdk.AccAddress, ibcPort string, msgs []wasmvmtypes.SubMsg) ([]byte, error) {
-					ctx.EventManager().EmitEvent(sdk.NewEvent(sdk.EventTypeMessage))
-					return nil, nil
-				}
-			},
-			expEvts: sdk.Events{},
-		},
 		"message emit non message events": {
 			setup: func(m *wasmtesting.MockMsgDispatcher) {
 				m.DispatchSubmessagesFn = func(ctx sdk.Context, contractAddr sdk.AccAddress, ibcPort string, msgs []wasmvmtypes.SubMsg) ([]byte, error) {

diff --git a/x/wasm/keeper/msg_dispatcher.go b/x/wasm/keeper/msg_dispatcher.go
@@ -99,10 +99,11 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk
 		}
 
 		// if it succeeds, commit state changes from submessage, and pass on events to Event Manager
+		var filteredEvents []sdk.Event
 		if err == nil {
 			commit()
-			ctx.EventManager().EmitEvents(em.Events())
-			ctx.EventManager().EmitEvents(events)
+			filteredEvents = filterEvents(append(em.Events(), events...))
+			ctx.EventManager().EmitEvents(filteredEvents)
 		} // on failure, revert state from sandbox, and ignore events (just skip doing the above)
 
 		// we only callback if requested. Short-circuit here the cases we don't want to
@@ -124,7 +125,7 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk
 			}
 			result = wasmvmtypes.SubcallResult{
 				Ok: &wasmvmtypes.SubcallResponse{
-					Events: sdkEventsToWasmVmEvents(events),
+					Events: sdkEventsToWasmVmEvents(filteredEvents),
 					Data:   responseData,
 				},
 			}
@@ -153,6 +154,17 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk
 	return rsp, nil
 }
 
+func filterEvents(events []sdk.Event) []sdk.Event {
+	// pre-allocate space for efficiency
+	res := make([]sdk.Event, 0, len(events))
+	for _, ev := range events {
+		if ev.Type != "message" {
+			res = append(res, ev)
+		}
+	}
+	return res
+}
+
 func sdkEventsToWasmVmEvents(events []sdk.Event) []wasmvmtypes.Event {
 	res := make([]wasmvmtypes.Event, len(events))
 	for i, ev := range events {

diff --git a/x/wasm/keeper/msg_dispatcher_test.go b/x/wasm/keeper/msg_dispatcher_test.go
@@ -85,6 +85,7 @@ func TestDispatchSubmessages(t *testing.T) {
 			}},
 			replyer: &mockReplyer{
 				replyFn: func(ctx sdk.Context, contractAddress sdk.AccAddress, reply wasmvmtypes.Reply) ([]byte, error) {
+					ctx.EventManager().EmitEvent(sdk.NewEvent("wasm-reply"))
 					return []byte("myReplyData"), nil
 				},
 			},
@@ -99,7 +100,9 @@ func TestDispatchSubmessages(t *testing.T) {
 			expEvents: []sdk.Event{{
 				Type:       "myEvent",
 				Attributes: []abci.EventAttribute{{Key: []byte("foo"), Value: []byte("bar")}},
-			}},
+			},
+				sdk.NewEvent("wasm-reply"),
+			},
 		},
 		"with context events - released on commit": {
 			msgs: []wasmvmtypes.SubMsg{{
@@ -266,6 +269,75 @@ func TestDispatchSubmessages(t *testing.T) {
 			expData:    []byte{},
 			expCommits: []bool{false, false},
 		},
+		"message event filtered without reply": {
+			msgs: []wasmvmtypes.SubMsg{{
+				ReplyOn: wasmvmtypes.ReplyNever,
+			}},
+			replyer: &mockReplyer{
+				replyFn: func(ctx sdk.Context, contractAddress sdk.AccAddress, reply wasmvmtypes.Reply) ([]byte, error) {
+					return nil, errors.New("should never be called")
+				},
+			},
+			msgHandler: &wasmtesting.MockMessageHandler{
+				DispatchMsgFn: func(ctx sdk.Context, contractAddr sdk.AccAddress, contractIBCPortID string, msg wasmvmtypes.CosmosMsg) (events []sdk.Event, data [][]byte, err error) {
+					myEvents := []sdk.Event{
+						sdk.NewEvent("message"),
+						sdk.NewEvent("execute", sdk.NewAttribute("foo", "bar")),
+					}
+					return myEvents, [][]byte{[]byte("myData")}, nil
+				},
+			},
+			expData:    nil,
+			expCommits: []bool{true},
+			expEvents:  []sdk.Event{sdk.NewEvent("execute", sdk.NewAttribute("foo", "bar"))},
+		},
+		"reply gets proper events": {
+			msgs: []wasmvmtypes.SubMsg{{ID: 1, ReplyOn: wasmvmtypes.ReplyAlways}},
+			replyer: &mockReplyer{
+				replyFn: func(ctx sdk.Context, contractAddress sdk.AccAddress, reply wasmvmtypes.Reply) ([]byte, error) {
+					if reply.Result.Err != "" {
+						return nil, errors.New(reply.Result.Err)
+					}
+					res := reply.Result.Ok
+
+					// ensure the input events are what we expect
+					// I didn't use require.Equal() to act more like a contract... but maybe that would be better
+					if len(res.Events) != 2 {
+						return nil, fmt.Errorf("event count: %#v", res.Events)
+					}
+					if res.Events[0].Type != "execute" {
+						return nil, fmt.Errorf("event0: %#v", res.Events[0])
+					}
+					if res.Events[1].Type != "wasm" {
+						return nil, fmt.Errorf("event1: %#v", res.Events[1])
+					}
+
+					// let's add a custom event here and see if it makes it out
+					ctx.EventManager().EmitEvent(sdk.NewEvent("wasm-reply"))
+
+					// update data from what we got in
+					return res.Data, nil
+				},
+			},
+			msgHandler: &wasmtesting.MockMessageHandler{
+				DispatchMsgFn: func(ctx sdk.Context, contractAddr sdk.AccAddress, contractIBCPortID string, msg wasmvmtypes.CosmosMsg) (events []sdk.Event, data [][]byte, err error) {
+					events = []sdk.Event{
+						sdk.NewEvent("message", sdk.NewAttribute("_contract_address", contractAddr.String())),
+						// we don't know what the contarctAddr will be so we can't use it in the final tests
+						sdk.NewEvent("execute", sdk.NewAttribute("_contract_address", "placeholder-random-addr")),
+						sdk.NewEvent("wasm", sdk.NewAttribute("random", "data")),
+					}
+					return events, [][]byte{[]byte("subData")}, nil
+				},
+			},
+			expData:    []byte("subData"),
+			expCommits: []bool{true},
+			expEvents: []sdk.Event{
+				sdk.NewEvent("execute", sdk.NewAttribute("_contract_address", "placeholder-random-addr")),
+				sdk.NewEvent("wasm", sdk.NewAttribute("random", "data")),
+				sdk.NewEvent("wasm-reply"),
+			},
+		},
 	}
 	for name, spec := range specs {
 		t.Run(name, func(t *testing.T) {

diff --git a/x/wasm/keeper/submsg_test.go b/x/wasm/keeper/submsg_test.go
@@ -93,30 +93,14 @@ func TestDispatchSubMsgSuccessCase(t *testing.T) {
 	require.NotNil(t, res.Result.Ok)
 	sub := res.Result.Ok
 	assert.Empty(t, sub.Data)
-	require.Len(t, sub.Events, 3)
+	require.Len(t, sub.Events, 1)
 
 	transfer := sub.Events[0]
 	assert.Equal(t, "transfer", transfer.Type)
 	assert.Equal(t, wasmvmtypes.EventAttribute{
 		Key:   "recipient",
 		Value: fred.String(),
 	}, transfer.Attributes[0])
-
-	sender := sub.Events[1]
-	assert.Equal(t, "message", sender.Type)
-	assert.Equal(t, wasmvmtypes.EventAttribute{
-		Key:   "sender",
-		Value: contractAddr.String(),
-	}, sender.Attributes[0])
-
-	// where does this come from?
-	module := sub.Events[2]
-	assert.Equal(t, "message", module.Type)
-	assert.Equal(t, wasmvmtypes.EventAttribute{
-		Key:   "module",
-		Value: "bank",
-	}, module.Attributes[0])
-
 }
 
 func TestDispatchSubMsgErrorHandling(t *testing.T) {
@@ -262,7 +246,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) {
 			submsgID: 5,
 			msg:      validBankSend,
 			// note we charge another 40k for the reply call
-			resultAssertions: []assertion{assertReturnedEvents(3), assertGasUsed(123000, 125000)},
+			resultAssertions: []assertion{assertReturnedEvents(1), assertGasUsed(116000, 121000)},
 		},
 		"not enough tokens": {
 			submsgID:    6,
@@ -282,7 +266,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) {
 			msg:      validBankSend,
 			gasLimit: &subGasLimit,
 			// uses same gas as call without limit
-			resultAssertions: []assertion{assertReturnedEvents(3), assertGasUsed(123000, 125000)},
+			resultAssertions: []assertion{assertReturnedEvents(1), assertGasUsed(116000, 121000)},
 		},
 		"not enough tokens with limit": {
 			submsgID:    16,
@@ -300,7 +284,6 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) {
 			// uses all the subGasLimit, plus the 92k or so for the main contract
 			resultAssertions: []assertion{assertGasUsed(subGasLimit+92000, subGasLimit+94000), assertErrorString("out of gas")},
 		},
-
 		"instantiate contract gets address in data and events": {
 			submsgID:         21,
 			msg:              instantiateContract,