[WIP] feat(gnovm, tm2): Yet Another Emit/Event

[notJoon]

Description

Trying to succeed in my predecessor's legacy.

Related Issue/PR

#575 emit & event built-in functions (@r3v4s) #853 feat: event & emit in gno (@anarcher) <- previous work. #975 [META] Gno Wishlist / Feature Request Dump (@zivkovicmilos)

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 38.88889% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 46.72%. Comparing base (7bf662a) to head (7a5fd74). Report is 5 commits behind head on master.

Files	Patch %	Lines
tm2/pkg/bft/abci/types/types.go	0.00%	7 Missing :warning:
gno.land/pkg/sdk/vm/keeper.go	60.00%	2 Missing :warning:
tm2/pkg/crypto/keys/client/maketx.go	0.00%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1653      +/-   ##
==========================================
+ Coverage   46.64%   46.72%   +0.08%     
==========================================
  Files         492      492              
  Lines       69624    69762     +138     
==========================================
+ Hits        32476    32597     +121     
- Misses      34442    34454      +12     
- Partials     2706     2711       +5     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[zivkovicmilos]

Will take a look at this and give feedback 🙏

[notJoon]

Will take a look at this and give feedback 🙏

Thank you @zivkovicmilos! I'm still trying to understand most things, every feedback would be greatly helpful

[zivkovicmilos]

@notJoon Looking at this today 🚀

[r3v4s]

@zivkovicmilos @notJoon I think we need few more test for this.

1. Single Block, Single Tx, Single Msg
2. Single Block, Single Tx, Multi Msg
3. Single Block, Multi Tx, Each Tx with Single Msg
4. Single Block, Multi Tx, Each Tx with Multi Msg

[r3v4s]

@notJoon off the top of my head, do you think it's possible to include not only pkg path but which function(i.e function name) triggered event??

If possible it brings huge advantage to what you said about transparency and traceability(https://github.com/gnolang/gno/issues/1833#issuecomment-2019490398).

[notJoon]

do you think it's possible to include not only pkg path but which function(i.e function name) triggered event??

I agree. Rather than just exposing the package name, it must be clearer to show the method name together.

Anyway we need to access the call stack to retrieve the last called function identifier. I think we can use the Frame type (i.e., Func.Name) to make it possible.

[1] https://github.com/gnolang/gno/blob/master/gnovm/pkg/gnolang/frame.go#L10-L31
[2] https://github.com/gnolang/gno/blob/master/gnovm/pkg/gnolang/values.go#L534

---

resolved

[notJoon]

The event messages that were previously being outputted have been improved to be displayed in JSON style. Especially when triggering multiple events, the previous version had poor readability, but with this modification, it has become easier to read.

Example

EVENTS: [{type: sender, pkgPath: gno.land/r/demo/ee, fn: Hello, timestamp: 1712968387, attributes: [key1: value1, key2: value2]}, {type: receiver, pkgPath: gno.land/r/demo/ee, fn: Hello, timestamp: 1712968387, attributes: [foo: bar]}]

[thehowl]

From the review meeting, let's use EventString instead of a custom, amino-registered GnoEvent.

The rationale is that we shouldn't pass on the wire events that are gno specific, and instead stick to a simple type already defined in TM2.

Inside the gnovm, in the implementation of std.Emit, use the EventString type and marshal the JSON data, placing the result within the EventString. Since you're not adding a new event type anymore, please revert the changes in abci/types/types.go.

[notJoon]

From the review meeting, let's use EventString instead of a custom, amino-registered GnoEvent.

In order to apply this change, I modified the existing Emit function. Instead of using the NewGnoEvent function that returned an sdk.Event, now use NewGnoEventString which returns abci.EventString. this new function do a marshaling to the event and store the its result.

Also, removed most of the code that was previously added to abci/types/types/go file. But, i kept the ResponseBase.EncodeEvent function to maintain the connection.

[notJoon]

I added a test that performs a very simple check 2e9d9ed, and I plan to add more cases later. For now just for saving.

[moul]

PS: @notJoon, could you please verbally confirm when you have resolved a conversation instead of using the "Resolve conversation" button?

[notJoon]

PS: @notJoon, could you please verbally confirm when you have resolved a conversation instead of using the "Resolve conversation" button?

I see. I'll do that next time.

[notJoon]

Please compare stdout using cmp stdout stdout.golden, as many other tests do.

Plase understand that I were unable to respond to that comment directly, so I create a separate thread for it.

Since cmp tests the results based on an exact match when comparing data, it is almost impossible to test mutable values like "Gas Used" or "Height". Because we cannot predict the values that comes next. Therefore, I believe it is sufficient to apply regex only to variable data while checking if the fixed value are accurately outputted.

> cmp stdout event1.golden
        diff stdout event1.golden
        --- stdout
        +++ event1.golden
        @@ -1,6 +1,5 @@
        -
         OK!
         GAS WANTED: 2500000
        -GAS USED:   2072886
        -HEIGHT:     433
        +GAS USED:   [0-9]+    // <- doesn't work to `\[0-9]+`, `\d+`, `d+` or just `GAS USED:` either.
        +HEIGHT:     [0-9]+
         EVENTS:     []

Original testscript Doc cmp part: [!] cmp file1 file2 Check that the named files have (or do not have) the same content. [...] File1 can be "stdout" or "stderr" to use the standard output or standard error from the most recent exec or wait command. (If the files have differing content and the command is not negated, the failure prints a diff.)

Of course, there might be other workaround methods if we look for them, but even if they are possible, I think the tests will unneccesarily become over-complicated. Therefore, I think it's sufficient to handle variable data with patterns using the current grep method and check if the rest of the strings (fixed values) are identical.

There should be other tests already using loadpkg.

Additionally, I didn't understand the meaning of the loadpkg part, so I left it unchanges. However, I'm not sure if it's actually necessary changes.

[dongwon8247]

@moul @thehowl Thank you guys for the thorough review on this. Could you please give a final review? thank you so much :pray:

[moul]

It looks great! Please address the latest comments, and we should be all set. Thank you!

[moul]

@leohhhn, how did you play with it? Sharing tips through text, a video, integration tests, or documentation would be great. It's good to show how people are getting started with this new feature.

[r3v4s]

FYI, for everyone who has been in this PR Please take a look at this bug #2028