Document event handling #581

ethanfrey · 2021-08-05T10:20:11Z

This goes into great detail on both how Tendermint and the Cosmos SDK handle events, as well as how we work with them in x/wasm and when interfacing with smart contracts.

This documents a spec on the desired format for implementing #440

Closes #448 and remove all filtering of message events.

Document a solution to #552 which is a small adjustment from the current behaviour.

Documents a format for #574

We may want to raise new issues for any other changes defined here if they don't fit in the above 3 issues.

ethanfrey · 2021-08-06T00:19:27Z

@alpe I took inspiration from your PR #583 and your issues, and went on to define the entire event system.
This is not (yet) cut in stone, so please make suggestions on how to improve it. Adding more fields to the custom execute etc events? Renaming code_id to _code_id? Injecting _code_id in all wasm events? etc...

This is one possible definition of how events work, let's adjust it to make it the best it can be.

ethanfrey · 2021-08-06T00:21:04Z

PD Does it make sense to use Go types when listing events? (It was a lot shorter than writing proper JSON all those places, which is why I did it, but maybe there is a better format we could use)

alpe

Excellent writeup and best doc on event internals I have seen in cosmos. ❤️

The Go type examples worked for me and expressed the scenarios very well. Don't worry, we can always change the doc in a new iteration.

alpe · 2021-08-06T09:15:34Z

EVENTS.md

+for all messages that it returned, which were later dispatched.
+
+Until 0.18.0, we stripped out the `message` events that were returned when dispatching `Response.messages`. However,
+they contain useful information to trace the topography of the call, especially when we call into native modules,


If all sdk modules would follow the distribution event design, I would totally agree on this approach. Unfortunately the bank module emits a message event in the keeper. Probably others as well.
We call the keeper when we transfer coins to the contract account. This would result into 2 different message events where only the wasm one is correct.
A more consistent approach was the message filtering so that we only track the message initiating the whole process via event. We do loose information going this way, but I would prefer this rather than emitting false positives.
We could also address the bank message issue by filtering out the bank message in our BankCoinTransferrer. For native modules calling the bank keeper this would not work though.

Afaik, we call the router for most dispatching (except ibc) and thus hit the message server as well.

I also hear your point about the non standard issue. But it is very useful to have a separator between the different sub messages.

How about this:

Filter out the inconsistent message event emitted by the module.

Always prepend a standard event type on the beginning of each submessage. We can define what it is, it is created by x/wasm and will be very consistent.

EVENTS.md

alpe · 2021-08-06T09:41:43Z

EVENTS.md

+address), it will emit a series of events that looks something like this:
+
+```go
+/// original execution (top-level message)


This list of events describe the process flow but as events are emitted after execution the actual order would be different. I think L321 would be the last event emitted when the original message was processed. L330 would be the first one when we handle the first contract response.

We can change what happens. I want to document the desired behaviour in here (which will be actual behaviour with the 0.18.0 tag), not the current behaviour.

We have control in x/wasm to do something like:

Keeper.execute called

call wasmvm.execute

emit execute message including the data from the response

emit wasm and wasm-* events from the contract

for each message in Response:

emit some "start sub message" event (if desired)

call the execution logic here (and emit all events like this, recursively - filtering message event)

emit some "stop sub message event"?

emit any other events we wish

As to the message: module=wasm, this is emitted in the message server and we can perform this any way we want to.
We can add it top the event manager before calling the keeper, or afterwards. I think placing it first (as in distribution) makes more sense than at the end (in bank).

What do you think the desired behaviour here is?

IMHO to emit the events in order of processing is better than what we do now. It should not matter but for debugging and flattening, I can see the benefit.

alpe · 2021-08-06T09:45:46Z

EVENTS.md

+    sdk.NewAttribute("result", hex.EncodeToString(initData)),
+)
+sdk.NewEvent(
+    "wasm",


Looking at this event with some distance now, I don't see how it adds value. Before the contract address was not available but it is now by the wasm operation event. Would be nice to drop this functionality and let people emit custom events when needed. That would be easier to explain in dev doc and no "empty" event would be emitted by default.

So, if I return non-empty attributes, then it will get this wasm event (tagged).
If I return empty attributes, it will not get this event, right?

The one issue I want to bring up (which came up from Secret network some time ago) is they want to be able to search for all actions on a contract easily. We can now search for execute._contract_address="foo". However, maybe we also want to include instantiate._contract_address="foo" and migrate._contract_address="foo". I guess all data is there.

So, yeah, if we say don't emit wasm event on empty attributes, I am fine with that

I would go one step further and drop the whole event. Why stick with a wasm event when we have custom events now for a contract? I don't see the use case for a client anymore. But there is complexity in wasmd to handle the case and complexity in wasm contracts for people to learn and understand the framework. Both sides would benefit from a clean design that does not support 2 ways of storing data in events.

90% of contracts just want to emit attributes.
Only a few emit multiple events.
We didn't want to make it that much more complex for all of them, which is why we didn't remove the attributes when we introduced events.

But you have a point. And with the new builder API from @uint we could even let people just add_attribute still and just expose them as a wasm-default event (so we keep the same cosmwasm-std api, but change the wasm interface between contract and runtime).

That is a larger discussion, and 0.16 is already cut. I feel it might be too late to make this change, but we could consider it for 1.0.0-beta.

That said, I would document both paths for now as we will support both in 0.18. If we change that for 1.0.0-beta, we can update the doc as well.

EVENTS.md

ethanfrey · 2021-08-06T10:43:18Z

I integrated feedback on the two clear suggestions (no _code_id injection and not emitted wasm event at all if no attributes in the Response).

Let's talk more about the use of message events and how to separate sub-events

ethanfrey

What do you think of this proposal?

If you like it, I can flesh it out with a more complex example, where we have a multi-level tree and both instantiate and reply dispatch messages to demonstrate the ordering here.

EVENTS.md

ethanfrey · 2021-08-09T08:49:56Z

@alpe I revised this and I think addressed you concerns in be3fcf1

Note that I would strongly like to mark every CosmWasm contract entry point called, as this can be very useful to understand a contract.

I also remove the (potentially very large) result attribute from the events 79f3fad as I see no need to return them now that we have better handling of the data response with submessages and can return a chosen value to the top level

alpe

Nice updates! 🌻 Two points left where we had disagreed:

Reply : I was focused on business app working with events but I could imagine that there is space for technical ones as well to expose internals like block explorers or IBC network graphs do. 🤷 So let's find some good attributes and do this
wasmvmtypes.EventAttribute : I do understand the value for Rust devs to create events easily but in the overall design there should not be 2 ways to create an event. Custom events are more powerful and should be preferred instead. Let's remove EventAttributes from the response 🙏 For the contract developers we could have a nice function to initialize a custom event with some defaults instead

alpe · 2021-08-09T10:07:08Z

EVENTS.md

+sdk.NewEvent(
+    "reply",
+    sdk.NewAttribute("_contract_addr", contractAddr.String()),
+    sdk.NewAttribute("success", strconv.FormatBool(err == nil)),


Can you elaborate what "success" means in this context? When the reply handling fails with an error we revert the TX.
To trace the replay call, we could store the submessage reply condition as an attribute: Always, Success, Error.

This should be renamed to give more clarity.

When reply is called, it receives the result of a sub message processing, which is either "Ok" or "Err".

If I have RelyOn::Always, is is useful to see if I am processing a success call, or handling an error callback.
The main use-case is for devs having some visibility.

Do you have an idea for a better name?

How about ("submsg", "ok") and ("submsg", "err") as the two options here? To show if we are processing a success case or handling an error.

Better suggestions welcome

how about: "mode": "handle_success" || "handle_failure"

codecov · 2021-08-09T11:58:36Z

Codecov Report

Merging #581 (e4987f7) into master (9efbb61) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #581      +/-   ##
==========================================
+ Coverage   59.70%   59.74%   +0.03%     
==========================================
  Files          45       45              
  Lines        5212     5212              
==========================================
+ Hits         3112     3114       +2     
+ Misses       1874     1873       -1     
+ Partials      226      225       -1

Impacted Files	Coverage Δ
x/wasm/keeper/keeper.go	`85.58% <0.00%> (+0.36%)`	⬆️

ethanfrey added this to the v0.18.0 milestone Aug 5, 2021

ethanfrey added 6 commits August 6, 2021 00:36

Start documenting events system

c9b7480

Fill out rest of the skeleton

f1ac7c0

Clarified event processing in the sdk

b343984

Import docs on bank / distribution modules

b71f726

Split x/wasm events into multiple types, document message concat

37d4105

_code_id, document emitting custom events, handling events in reply

90d73a3

ethanfrey force-pushed the events-docs branch from 657b427 to 90d73a3 Compare August 6, 2021 00:12

ethanfrey marked this pull request as ready for review August 6, 2021 00:13

ethanfrey requested a review from alpe as a code owner August 6, 2021 00:13

ethanfrey mentioned this pull request Aug 6, 2021

Some examples/comments #583

Closed

alpe reviewed Aug 6, 2021

View reviewed changes

ethanfrey added 2 commits August 6, 2021 12:37

Remove all _code_id references, no injection in wasm events

33d58d5

Empty Response.attributes -> no wasm event emitted

3c920a1

ethanfrey commented Aug 6, 2021

View reviewed changes

EVENTS.md Show resolved Hide resolved

ethanfrey added 2 commits August 9, 2021 10:46

Remove submsg_start/stop, improve documentation of flattening

be3fcf1

Remove hex-encoded result attribute

79f3fad

ethanfrey mentioned this pull request Aug 9, 2021

Multitest events match wasmd CosmWasm/cw-plus#380

Merged

5 tasks

alpe reviewed Aug 9, 2021

View reviewed changes

Use mode: handle_failure/success attribute on reply

e4987f7

alpe approved these changes Aug 9, 2021

View reviewed changes

ethanfrey merged commit 5912902 into master Aug 9, 2021

ethanfrey deleted the events-docs branch August 9, 2021 12:55

ethanfrey mentioned this pull request Aug 11, 2021

Events/ event attributes on reply #552

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document event handling #581

Document event handling #581

ethanfrey commented Aug 5, 2021 •

edited

Loading

ethanfrey commented Aug 6, 2021 •

edited

Loading

ethanfrey commented Aug 6, 2021

alpe left a comment •

edited

Loading

alpe Aug 6, 2021

ethanfrey Aug 6, 2021

alpe Aug 6, 2021

ethanfrey Aug 6, 2021

ethanfrey Aug 6, 2021

alpe Aug 6, 2021

alpe Aug 6, 2021

ethanfrey Aug 6, 2021

alpe Aug 6, 2021

ethanfrey Aug 6, 2021

ethanfrey commented Aug 6, 2021

ethanfrey left a comment

ethanfrey commented Aug 9, 2021

alpe left a comment

alpe Aug 9, 2021

ethanfrey Aug 9, 2021

ethanfrey Aug 9, 2021

alpe Aug 9, 2021

codecov bot commented Aug 9, 2021

Document event handling #581

Document event handling #581

Conversation

ethanfrey commented Aug 5, 2021 • edited Loading

ethanfrey commented Aug 6, 2021 • edited Loading

ethanfrey commented Aug 6, 2021

alpe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Aug 6, 2021

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey commented Aug 9, 2021

alpe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2021

Codecov Report

ethanfrey commented Aug 5, 2021 •

edited

Loading

ethanfrey commented Aug 6, 2021 •

edited

Loading

alpe left a comment •

edited

Loading