feat(tm2/gnovm): add msg_idx in event to identify #2061

r3v4s · 2024-05-09T06:02:53Z

Closes #2031

As describe in above issue, this pr adds msg_idx field in ctx.Event struct to identify which event was emitted by which msg.

FYI, this pr also includes another bug fix in #2030. It can be close if we're willing to merge this one

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

codecov · 2024-05-09T06:12:54Z

Codecov Report

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

Project coverage is 46.25%. Comparing base (8057505) to head (dabf13c).
Report is 9 commits behind head on master.

Files	Patch %	Lines
gno.land/pkg/gnoclient/client_txs.go	57.44%	10 Missing and 10 partials ⚠️
tm2/pkg/bft/abci/types/types.go	0.00%	4 Missing ⚠️
tm2/pkg/sdk/baseapp.go	60.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2061      +/-   ##
==========================================
- Coverage   54.96%   46.25%   -8.72%     
==========================================
  Files         481      505      +24     
  Lines       67391    71040    +3649     
==========================================
- Hits        37040    32856    -4184     
- Misses      27330    35457    +8127     
+ Partials     3021     2727     -294

Flag	Coverage Δ
gno.land	`61.65% <57.44%> (?)`
tm2	`53.94% <42.85%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

moul · 2024-05-12T09:57:41Z

Why do you need an index when the events are delivered in an ordered slice?

Your PR only shows msg_idx=0, which seems incorrect.

r3v4s · 2024-05-13T01:03:23Z

Why do you need an index when the events are delivered in an ordered slice?

Your PR only shows msg_idx=0, which seems incorrect.

Index is about MESSAGE index, not EVENT. So as described in #2031, current event output is just list of events without knowing which msg emitted which event if tx inlcudes 2 or more msgs.

I'll add test cases that runs 2 messages // UPDATE 3002936

ajnavarro · 2024-05-13T09:47:03Z

gno.land/pkg/gnoclient/integration_test.go

@@ -113,6 +114,64 @@ func TestCallMultiple_Integration(t *testing.T) {
 	assert.Equal(t, expected, got)
 }

+func TestCallMultipleWithEvent_Integration(t *testing.T) {


Could you test generating several events between different blocks and different transactions? thanks

I'll add test that tests multi tx + mutli msg.

But I don't think testing events between different block is necessary test, different block really has nothing to do with multi-msg

ajnavarro · 2024-05-13T09:47:59Z

gnovm/stdlibs/std/emit_event.go

+func (e gnoEvent) SetMsgIdx(msgIdx int) interface{} {
+	e.MsgIdx = msgIdx
+	return e
+}


Why this instead of e.MsgIdx = msgIdx when needed?

~~If what you're suggesting is e.MsgIdx = msgIdx without return e, vm was panicking if function is like func (e *gnoEvnet).~~

UPDATE //
abci.Event is interface type described here

Based on my knowledge pointer receiver can't be used with interface

ajnavarro · 2024-05-13T09:50:32Z

tm2/pkg/bft/abci/types/types.go

@@ -206,6 +206,7 @@ type Error interface {

 type Event interface {
 	AssertABCIEvent()
+	SetMsgIdx(int) interface{}


If you want to force that an Event needs an ID, I would recommend adding the method providing that ID instead of the provided Setter:

Suggested change

SetMsgIdx(int) interface{}

ID() []byte

If the ID is expected to be sequential:

Suggested change

SetMsgIdx(int) interface{}

Seq() uint64

Looking Good!

Reverted.
Event needs not only ID, but also setter for setting msg_idx.

Currently, event gets emitted from here

gno/gnovm/stdlibs/std/emit_event.go

Lines 14 to 31 in d7c04be

func X_emit(m *gno.Machine, typ string, attrs []string) {

eventAttrs, err := attrKeysAndValues(attrs)

if err != nil {

m.Panic(typedString(err.Error()))

}

pkgPath := CurrentRealmPath(m)

fnIdent := getPrevFunctionNameFromTarget(m, "Emit")

evt := gnoEvent{

Type: typ,

PkgPath: pkgPath,

Func: fnIdent,

Attributes: eventAttrs,

}

ctx := m.Context.(ExecContext)

ctx.EventLogger.EmitEvent(evt)

}

At this point, there is no way for us to determine message index if current request was made by multi-msg tx.

However baseapp.go does know full list of messages so, we can set message index from there.

gno/tm2/pkg/sdk/baseapp.go

Lines 634 to 666 in dabf13c

for i, msg := range msgs {

// match message route

msgRoute := msg.Route()

handler := app.router.Route(msgRoute)

if handler == nil {

result.Error = ABCIError(std.ErrUnknownRequest("unrecognized message type: " + msgRoute))

return

}

var msgResult Result

ctx = ctx.WithEventLogger(NewEventLogger())

// run the message!

// skip actual execution for CheckTx mode

if mode != RunTxModeCheck {

msgResult = handler.Process(ctx, msg)

}

// Each message result's Data must be length prefixed in order to separate

// each result.

data = append(data, msgResult.Data...)

// each msgs' event

var msgEvent []abci.Event

// handle each msg's ctx event

msgCtxEvents := ctx.EventLogger().Events()

for _, msgCtxEvent := range msgCtxEvents {

overwriteInterface := msgCtxEvent.SetMsgIdx(i)

overwrwriteEvent := overwriteInterface.(abci.Event)

msgEvent = append(msgEvent, overwrwriteEvent)

}

zivkovicmilos

Please check my comments, I don't think we need this functionality in the events 🙁

zivkovicmilos · 2024-05-16T08:26:22Z

gno.land/pkg/gnoclient/client_txs.go

@@ -114,6 +114,62 @@
 	return c.signAndBroadcastTxCommit(tx, cfg.AccountNumber, cfg.SequenceNumber)
 }

+// CallTxSync executes one or more MsgCall calls on the blockchain synchronously
+func (c *Client) CallTxSync(cfg BaseTxCfg, msgs ...MsgCall) (*ctypes.ResultBroadcastTx, error) {


We should leave this for a separate PR 🙏

zivkovicmilos · 2024-05-16T08:29:16Z

tm2/pkg/sdk/baseapp.go

-		// TODO append msgevent from ctx. XXX XXX
+
+		// each msgs' event
+		var msgEvent []abci.Event


You know at this point in time how many events you'll have, so this slice should be initialized with a capacity:

msgEvent := make([]abci.Event, 0, len(msgCtxEvents))

zivkovicmilos · 2024-05-16T08:30:38Z

gnovm/stdlibs/std/emit_event.go

 	Attributes []gnoEventAttribute `json:"attrs"`
 }

 func (e gnoEvent) AssertABCIEvent() {}

+func (e gnoEvent) SetMsgIdx(msgIdx int) interface{} {


Why do you need to return the message event from this method?

That type gno Event will be emitted from here

gno/gnovm/stdlibs/std/emit_event.go

Line 31 in dabf13c

ctx.EventLogger.EmitEvent(evt)

Which requires type should be abci.Event

gno/tm2/pkg/sdk/events.go

Lines 23 to 37 in ccc6d5b

// EmitEvent stores a single Event object.

func (em *EventLogger) EmitEvent(event Event) {

em.events = append(em.events, event)

}

// EmitEvents stores a series of Event objects.

func (em *EventLogger) EmitEvents(events []Event) {

em.events = append(em.events, events...)

}

// ----------------------------------------------------------------------------

// Event

// ----------------------------------------------------------------------------

type Event = abci.Event

And also abci.Evnet is interface

gno/tm2/pkg/bft/abci/types/types.go

Lines 207 to 210 in dabf13c

type Event interface {

AssertABCIEvent()

SetMsgIdx(int) interface{}

}

Based my knowledge, to modify struct value using setter without return value it need pointer receiver. But we can't use pointer receiver for interface type.

zivkovicmilos · 2024-05-16T08:33:51Z

tm2/pkg/sdk/baseapp.go

+
+		// handle each msg's ctx event
+		msgCtxEvents := ctx.EventLogger().Events()
+		for _, msgCtxEvent := range msgCtxEvents {


I'm not sure what kind of performance overhead this has, but it can't be minimal

zivkovicmilos · 2024-05-16T08:40:21Z

tm2/pkg/bft/abci/types/types.go

@@ -206,6 +206,7 @@ type Error interface {

 type Event interface {
 	AssertABCIEvent()
+	SetMsgIdx(int) interface{}


I'm gonna suggest something radical, but hear me out

We don't actually need this information in the event.
Why would it matter which event belongs to which message?

How can you guarantee that the node doesn't shuffle the messages before executing them? (it doesn't now, but I see no reason why it might not)

I'm not in favor of this idea because it:

introduces more complexity on the already bloated event API

it's just a QoL feature that doesn't have any real functional value, since events are async and we should be concerned with their content, not their origin

We don't actually need this information in the event.
Why would it matter which event belongs to which message?

Let't say... single block has N txs, and each txs has N2 messages and each message has N3 emitted events.

If all of N2 messages are same function with just different input params, this is situation where I use msg_idx to identify it.

One of gnoswap(+interface) functionality is removing all of your positions in one shot, by calling RemovePosition(positionId) several time(≈several msgs).

If that RemovePosition emits few event with some underlying infos. At this point, we need to know which events was came from which message (i.e which position emitted which events)

How can you guarantee that the node doesn't shuffle the messages before executing them? (it doesn't now, but I see no reason why it might not)

I really didn't think about this :(

introduces more complexity on the already bloated event API

Agreed

If all of N2 messages are same function with just different input params, this is situation where I use msg_idx to identify it.

Yes, but why? Isn't it enough that you can parse RemovePosition events on the client, and use data from the event content to figure out what to do?

Maybe I'm missing something, but tying events to their respective messages doesn't solve any issue -- you should be able to adequately parse them client-side (reactive, async) and execute logic based on their appearance. This is the foundation of event driven design

@ajnavarro @gfanton maybe I am missing something?

zivkovicmilos · 2024-06-11T08:46:25Z

After our PR discussions, and discussions with @r3v4s, we've decided to close this PR. The bulk of the reasoning is outlined here:
#2061 (comment)

feat: add msg idx in event

1cc8c5c

r3v4s requested review from thehowl, jaekwon, moul, piux2 and zivkovicmilos as code owners May 9, 2024 06:02

github-actions bot assigned r3v4s May 9, 2024

github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 🌐 tendermint v2 Issues or PRs tm2 related labels May 9, 2024

r3v4s changed the title ~~[tm2/gnovm] feat: add msg_idx in event to identify~~ feat(tm2/gnovm): add msg_idx in event to identify May 9, 2024

fix: test cases

bd6b483

r3v4s requested a review from a team as a code owner May 9, 2024 06:19

github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label May 9, 2024

fix: txtar test

03447f2

Kouteki added this to the 🏗4️⃣ test4.gno.land milestone May 10, 2024

test: add multi-msg event test

3002936

r3v4s requested review from leohhhn, gfanton and a team as code owners May 13, 2024 03:32

github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label May 13, 2024

ajnavarro requested changes May 13, 2024

View reviewed changes

r3v4s marked this pull request as draft May 13, 2024 13:21

r3v4s added 2 commits May 14, 2024 14:31

feat: client TxSync

cae1fee

test: multiple txs, multiple msgs, multiple events

dabf13c

r3v4s force-pushed the feat/add-msg-idx-in-event branch from df0f268 to dabf13c Compare May 14, 2024 05:34

r3v4s marked this pull request as ready for review May 14, 2024 05:49

zivkovicmilos requested changes May 16, 2024

View reviewed changes

zivkovicmilos closed this Jun 11, 2024

zivkovicmilos mentioned this pull request Jun 11, 2024

[tm2/gnovm] Identify each msg's event #2031

Closed

jinoosss mentioned this pull request Jun 25, 2024

Track and identify emitted events #2437

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tm2/gnovm): add msg_idx in event to identify #2061

feat(tm2/gnovm): add msg_idx in event to identify #2061

r3v4s commented May 9, 2024 •

edited

Loading

codecov bot commented May 9, 2024 •

edited

Loading

moul commented May 12, 2024

r3v4s commented May 13, 2024 •

edited

Loading

ajnavarro May 13, 2024

r3v4s May 13, 2024

ajnavarro May 13, 2024

r3v4s May 13, 2024 •

edited

Loading

ajnavarro May 13, 2024

r3v4s May 13, 2024

r3v4s May 14, 2024

zivkovicmilos left a comment

zivkovicmilos May 16, 2024

r3v4s May 16, 2024

zivkovicmilos May 16, 2024

zivkovicmilos May 16, 2024

r3v4s May 16, 2024

zivkovicmilos May 16, 2024

zivkovicmilos May 16, 2024

r3v4s May 16, 2024

r3v4s May 16, 2024

zivkovicmilos May 17, 2024

zivkovicmilos commented Jun 11, 2024

	func X_emit(m *gno.Machine, typ string, attrs []string) {
	eventAttrs, err := attrKeysAndValues(attrs)
	if err != nil {
	m.Panic(typedString(err.Error()))
	}

	pkgPath := CurrentRealmPath(m)
	fnIdent := getPrevFunctionNameFromTarget(m, "Emit")

	evt := gnoEvent{
	Type: typ,
	PkgPath: pkgPath,
	Func: fnIdent,
	Attributes: eventAttrs,
	}
	ctx := m.Context.(ExecContext)
	ctx.EventLogger.EmitEvent(evt)
	}

	for i, msg := range msgs {
	// match message route
	msgRoute := msg.Route()
	handler := app.router.Route(msgRoute)
	if handler == nil {
	result.Error = ABCIError(std.ErrUnknownRequest("unrecognized message type: " + msgRoute))
	return
	}

	var msgResult Result
	ctx = ctx.WithEventLogger(NewEventLogger())

	// run the message!
	// skip actual execution for CheckTx mode
	if mode != RunTxModeCheck {
	msgResult = handler.Process(ctx, msg)
	}

	// Each message result's Data must be length prefixed in order to separate
	// each result.
	data = append(data, msgResult.Data...)

	// each msgs' event
	var msgEvent []abci.Event

	// handle each msg's ctx event
	msgCtxEvents := ctx.EventLogger().Events()
	for _, msgCtxEvent := range msgCtxEvents {
	overwriteInterface := msgCtxEvent.SetMsgIdx(i)
	overwrwriteEvent := overwriteInterface.(abci.Event)

	msgEvent = append(msgEvent, overwrwriteEvent)
	}

	// EmitEvent stores a single Event object.
	func (em *EventLogger) EmitEvent(event Event) {
	em.events = append(em.events, event)
	}

	// EmitEvents stores a series of Event objects.
	func (em *EventLogger) EmitEvents(events []Event) {
	em.events = append(em.events, events...)
	}

	// ----------------------------------------------------------------------------
	// Event
	// ----------------------------------------------------------------------------

	type Event = abci.Event

	type Event interface {
	AssertABCIEvent()
	SetMsgIdx(int) interface{}
	}

feat(tm2/gnovm): add msg_idx in event to identify #2061

feat(tm2/gnovm): add msg_idx in event to identify #2061

Conversation

r3v4s commented May 9, 2024 • edited Loading

codecov bot commented May 9, 2024 • edited Loading

Codecov Report

moul commented May 12, 2024

r3v4s commented May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r3v4s May 13, 2024 • 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

zivkovicmilos 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

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

zivkovicmilos commented Jun 11, 2024

r3v4s commented May 9, 2024 •

edited

Loading

codecov bot commented May 9, 2024 •

edited

Loading

r3v4s commented May 13, 2024 •

edited

Loading

r3v4s May 13, 2024 •

edited

Loading