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

Ensure proper events sent in reply #587

Closed
ethanfrey opened this issue Aug 11, 2021 · 3 comments · Fixed by #589
Closed

Ensure proper events sent in reply #587

ethanfrey opened this issue Aug 11, 2021 · 3 comments · Fixed by #589
Milestone

Comments

@ethanfrey
Copy link
Member

In EVENTS.md we define not only the events emitted by the chain but the events that are passed into reply blocks.

Note: you can use the reflect contract to test this, it will just store what it gets in the reply block into it's local storage to be queried later.

This is follow up from #586 (comment)

In #586, we filter out all message type events after dispatching all submessages However, I would argue that we must filter them before returning the events to the reply block, which is what is in the spec.

Looking into the internals (and assuming the default implementation... all the interfaces make flow analysis a bit tricky), we wee how we already make a temporary event manager before dispatching the message.

When then only re-emit them if the event succeeds, which avoids errors from poluting the event stack. Interestingly, there seem to be two sources of events and I am unclear what goes where:

if err == nil {
	commit()
	ctx.EventManager().EmitEvents(em.Events())
	ctx.EventManager().EmitEvents(events)
} // on failure, revert state from sandbox, and ignore events (just skip doing the above)

Which events are in events and which in em.Events(). Can we drop em.Events()?

Later on we see that only the events are passed into the reply block on success.

We should clear this up. I would propose the following:

  • Verify what is coming from the two event sources (events and em.Events())
  • Combine them (or drop one of them) and filter out message type events to produce filteredEvents.
  • ctx.EventManager().EmitEvents(filteredEvents) instead of these two calls
  • pass in filteredEvents to the reply call

This will ensure no messages make it to reply. There are never dups (between the two event sources) and it removes the need to filter later in Handle

@ethanfrey ethanfrey added this to the v0.18.0 milestone Aug 11, 2021
@ethanfrey ethanfrey mentioned this issue Aug 11, 2021
3 tasks
@alpe
Copy link
Contributor

alpe commented Aug 11, 2021

Good finding. The events on reply are indeed not as defined in the spec.

The current flow is:

# keeper.instantiate/ execute/ migrate/ sudo/...  contract response handling:
	-> keeper.handleContractResponse (keeper.go L840)
	  -> DefaultWasmVMContractResponseHandler.Handle (keeper.go L1074)  (filters out message events)
	  	 -> MessageDispatcher.DispatchSubmessages  (calls into sdk router and our handlers, where new events can be emitted and are not filtered out for `reply`)

Which events are in events and which in em.Events(). Can we drop em.Events()?

  • events are returned from sdk.router processing messages via sdk.Result.
  • em.Events() are emitted from calling keepers in our handler_plugin

I agree to combining both types to make it easier.
It would have been nice to decorate the EventManager in ctx to do the filtering but it is typed to a struct. So it needs to go into DispatchSubmessages and other places...

@ethanfrey
Copy link
Member Author

It would have been nice to decorate the EventManager in ctx to do the filtering but it is typed to a struct. So it needs to go into DispatchSubmessages and other places...

I agree. I had that thought as well, but no interface :(

So, we could combine em.Events() and events, then filter them, then pass them out?

I'd be happy to write some tests here and implement it. (especially if you want to pick up confio/tgrade#85)

@alpe
Copy link
Contributor

alpe commented Aug 12, 2021

I had started some work yesterday already to combine the events. Let me bring this to a stable state and I would be happy if you could add a reflect test to confirm it's done as defined in the spec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants