Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter events before reply block #589

Merged
merged 6 commits into from
Aug 12, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions x/wasm/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
9 changes: 0 additions & 9 deletions x/wasm/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
25 changes: 22 additions & 3 deletions x/wasm/keeper/msg_dispatcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
},
}
Expand Down Expand Up @@ -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)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 You can reduce complexity of this method a lot by just accepting []sdk.Event. On the caller side just do: filterEvents(append(em.Events(), events...)). I would not expect thousands of events that require some optimization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, in Rust everytime you heap allocate to make vectors (slices), it is considered "expensive". I guess we do not micro-optimize so aggressively in Go.

I will simplify

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 {
Expand Down
74 changes: 73 additions & 1 deletion x/wasm/keeper/msg_dispatcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
},
Expand All @@ -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{{
Expand Down Expand Up @@ -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) {
Expand Down
23 changes: 3 additions & 20 deletions x/wasm/keeper/submsg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down