From 35cc9027d87c3b1aa0be1042fa0ab898fc7537ca Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 12 Aug 2021 13:15:29 +0200 Subject: [PATCH 1/6] Add failing testcase showing messages make it into reply block --- x/wasm/keeper/keeper.go | 1 + x/wasm/keeper/msg_dispatcher_test.go | 52 +++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go index 020c2b0838..438059bd9b 100644 --- a/x/wasm/keeper/keeper.go +++ b/x/wasm/keeper/keeper.go @@ -1080,6 +1080,7 @@ func (h DefaultWasmVMContractResponseHandler) Handle(ctx sdk.Context, contractAd case rsp != nil: result = rsp } + // TODO: remove this - handled inside DispatchSubmessages // emit non message type events only for _, e := range em.Events() { if e.Type != sdk.EventTypeMessage { diff --git a/x/wasm/keeper/msg_dispatcher_test.go b/x/wasm/keeper/msg_dispatcher_test.go index 94d6f322ad..fc3f73fb4a 100644 --- 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,53 @@ func TestDispatchSubmessages(t *testing.T) { expData: []byte{}, expCommits: []bool{false, false}, }, + "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[0].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) { From 29d5e29dc00414268a7922bc278732e651c841f0 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 12 Aug 2021 13:22:36 +0200 Subject: [PATCH 2/6] One more test for message event without reply --- x/wasm/keeper/msg_dispatcher_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/x/wasm/keeper/msg_dispatcher_test.go b/x/wasm/keeper/msg_dispatcher_test.go index fc3f73fb4a..03afce7de2 100644 --- a/x/wasm/keeper/msg_dispatcher_test.go +++ b/x/wasm/keeper/msg_dispatcher_test.go @@ -269,6 +269,28 @@ 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{ From f547bf66300df42fcfa913d8551c5f4d13b09154 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 12 Aug 2021 13:28:58 +0200 Subject: [PATCH 3/6] Filter messages, fix new tests, break older ones --- x/wasm/keeper/msg_dispatcher.go | 25 ++++++++++++++++++++++--- x/wasm/keeper/msg_dispatcher_test.go | 2 +- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/x/wasm/keeper/msg_dispatcher.go b/x/wasm/keeper/msg_dispatcher.go index 18e774ac86..79071e57ef 100644 --- 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(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,24 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk return rsp, nil } +func filterEvents(events ...[]sdk.Event) []sdk.Event { + // pre-allocate space for efficiency + cap := 0 + for _, evts := range events { + cap += len(evts) + } + res := make([]sdk.Event, 0, cap) + + for _, evts := range events { + for _, ev := range evts { + 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 index 03afce7de2..42f9eab0c4 100644 --- a/x/wasm/keeper/msg_dispatcher_test.go +++ b/x/wasm/keeper/msg_dispatcher_test.go @@ -308,7 +308,7 @@ func TestDispatchSubmessages(t *testing.T) { if res.Events[0].Type != "execute" { return nil, fmt.Errorf("event0: %#v", res.Events[0]) } - if res.Events[0].Type != "wasm" { + if res.Events[1].Type != "wasm" { return nil, fmt.Errorf("event1: %#v", res.Events[1]) } From 514985c4f1602b204edc9d33b3ed0090e2be08b9 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 12 Aug 2021 13:33:23 +0200 Subject: [PATCH 4/6] Update tests to not expect message event --- x/wasm/keeper/submsg_test.go | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/x/wasm/keeper/submsg_test.go b/x/wasm/keeper/submsg_test.go index 820d4e1349..11c1647ced 100644 --- a/x/wasm/keeper/submsg_test.go +++ b/x/wasm/keeper/submsg_test.go @@ -93,7 +93,7 @@ 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) @@ -101,22 +101,6 @@ func TestDispatchSubMsgSuccessCase(t *testing.T) { 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, From f51ec6709972abbceb12d5eb7ab21500bf521af5 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 12 Aug 2021 13:36:22 +0200 Subject: [PATCH 5/6] Remove (now-unneeded) filtering in DefaultWasmVMContractResponseHandler.Handle --- x/wasm/keeper/keeper.go | 10 +--------- x/wasm/keeper/keeper_test.go | 9 --------- 2 files changed, 1 insertion(+), 18 deletions(-) diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go index 438059bd9b..2448122850 100644 --- a/x/wasm/keeper/keeper.go +++ b/x/wasm/keeper/keeper.go @@ -1072,20 +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 } - // TODO: remove this - handled inside DispatchSubmessages - // 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 index 041a991cc4..0cf5628ff7 100644 --- 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) { From 7fbf513d71de82dc2a3a7ea3ce729371e7188bd2 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 12 Aug 2021 14:49:43 +0200 Subject: [PATCH 6/6] Simplify filter method --- x/wasm/keeper/msg_dispatcher.go | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/x/wasm/keeper/msg_dispatcher.go b/x/wasm/keeper/msg_dispatcher.go index 79071e57ef..7ed63b1729 100644 --- a/x/wasm/keeper/msg_dispatcher.go +++ b/x/wasm/keeper/msg_dispatcher.go @@ -102,7 +102,7 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk var filteredEvents []sdk.Event if err == nil { commit() - filteredEvents = filterEvents(em.Events(), 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) @@ -154,19 +154,12 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk return rsp, nil } -func filterEvents(events ...[]sdk.Event) []sdk.Event { +func filterEvents(events []sdk.Event) []sdk.Event { // pre-allocate space for efficiency - cap := 0 - for _, evts := range events { - cap += len(evts) - } - res := make([]sdk.Event, 0, cap) - - for _, evts := range events { - for _, ev := range evts { - if ev.Type != "message" { - res = append(res, ev) - } + res := make([]sdk.Event, 0, len(events)) + for _, ev := range events { + if ev.Type != "message" { + res = append(res, ev) } } return res