feat: implement EventManager().GetABCIEventHistory() #9305

michaelfig · 2021-05-11T23:16:22Z

Description

Experimental PR to keep this block's event history in EventManager() so that message handlers and end blockers can use the events to make decisions without having to explicitly hook into other modules.

Some of the initial discussion as to the general structure of the PR is at the thread at tendermint/tendermint#5977 (comment).

closes: #XXX

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

tac0turtle · 2021-05-20T10:29:54Z

Hey, thanks for opening this PR. It seems there are some considerations that need to be discussed prior to making a PR. There is a discussion (#8272) that touches on this. Lets continue the conversation there.

michaelfig · 2021-05-22T02:43:11Z

@marbar3778 I'd like to reopen this PR and evolve it based on the thread linked above (tendermint/tendermint#5977 (comment)) where @alexanderbez and @robert-zaremba also weighed in that this design might be feasible.

I know there's significantly more work to be done before this PR can be merged, but Agoric has a vested interest, and @JimLarson and I can put in the effort needed to refine it.

tac0turtle · 2021-05-22T10:04:48Z

Yea, feel free to reopen. Glad to see there is a design to move forward with.

michaelfig · 2021-05-22T20:01:51Z

Yea, feel free to reopen. Glad to see there is a design to move forward with.

I don't see a button to reopen this PR. Could you please do it for me?

github-actions · 2021-07-08T00:02:27Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

michaelfig · 2021-07-08T04:33:25Z

Hi @JimLarson, can you please review this change?

technicallyty

just a couple concerns/comments - would be nice to have some tests to show it is collecting event history in a block properly

technicallyty · 2021-08-06T20:01:01Z

baseapp/baseapp.go

@@ -751,6 +756,7 @@ func (app *BaseApp) runMsgs(ctx sdk.Context, msgs []sdk.Msg, mode runTxMode) (*s
 		// Note: Each message result's data must be length-prefixed in order to
 		// separate each result.
 		events = events.AppendEvents(msgEvents)
+		historicalEvents = append(historicalEvents, msgEvents.ToABCIEvents()...)


does this need to be used somewhere? it looks like its collecting all the msgs events, but its not given to anything else?

technicallyty · 2021-08-06T20:02:33Z

baseapp/state.go

-	ctx sdk.Context
+	ms           sdk.CacheMultiStore
+	ctx          sdk.Context
+	eventHistory []abci.Event


is the extra field here truly necessary? im curious if this PR can work by just storing the events in the state's existing ctx event manager.

technicallyty · 2021-08-06T20:06:30Z

types/events.go

+
+// GetEventHistory returns a deep copy of the ABCI events that have been
+// committed to thus far.
+func (em *EventManager) GetABCIEventHistory() []abci.Event {


the history is stored as a value, not a pointer - not sure if deep copy is needed here, couldn't we just return em.history ?

aaronc · 2021-08-06T21:59:25Z

This definitely would need to be discussed in an architecture call. I have proposed #9656 as an explicit way to have event hooks.

One issue with this approach is that there currently is no formal contract that events are part of the state machine or have any kind of breaking API guarantees. #9656 would add some explicit typing. Either way we do need to consider what the most correct way to do this is and the types of guarantees modules need to provide each other around events.

michaelfig · 2021-08-24T04:39:05Z

This definitely would need to be discussed in an architecture call. I have proposed #9656 as an explicit way to have event hooks.

@JimLarson would you be able to help work on this in the architecture call when it comes up, as you know what our constraints are?

Essentially, this PR is the result of needing something urgently to solve a problem that otherwise would have required hacking up a modified x/bank module. I'd be very happy if something more robust and coherent could be arrived at, and then Agoric would transition our uses of this PR to that new thing, whatever it is.

One issue with this approach is that there currently is no formal contract that events are part of the state machine or have any kind of breaking API guarantees. #9656 would add some explicit typing. Either way we do need to consider what the most correct way to do this is and the types of guarantees modules need to provide each other around events.

Agreed.

The main advantage of the current PR is that it is conceptually simple and just provides additional information to modules that run later in the block. I would hope the replacement (probably more general) solution could preserve that spirit.

Thanks for your attention to this!

github-actions · 2021-10-09T00:02:45Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

tac0turtle · 2021-10-13T10:53:05Z

@michaelfig is it okay we close this and work on event hooks? It does not seem like this will be merged.

robert-zaremba · 2022-01-24T16:47:25Z

One question: is anyone using this approach in a fork?

michaelfig · 2022-01-24T17:53:14Z

One question: is anyone using this approach in a fork?

Agoric is in devnet and for a future mainnet phase.

robert-zaremba · 2022-01-28T19:04:09Z

During today architecture call we decided to move forward with a canonical event hooks implementation in the Cosmos SDK. We are more leaning to #9656 design.
Looking forward for seeing people who will like to contribute

feat: implement EventManager().GetABCIEventHistory()

9a04694

michaelfig mentioned this pull request May 11, 2021

PLACEHOLDER feat: implement EventManager().GetABCIEventHistory() agoric-labs/cosmos-sdk#45

Closed

9 tasks

tac0turtle closed this May 20, 2021

tac0turtle reopened this May 23, 2021

michaelfig mentioned this pull request May 28, 2021

Upgrade and use agoric-labs/cosmos-sdk fork Agoric/agoric-sdk#3198

Closed

5 tasks

github-actions bot added the stale label Jul 8, 2021

michaelfig marked this pull request as ready for review July 8, 2021 04:34

michaelfig requested review from aaronc and alexanderbez as code owners July 8, 2021 04:34

Merge branch 'master' into mfig-context-event-history

1057881

amaury1093 removed the stale label Jul 13, 2021

amaury1093 requested a review from robert-zaremba July 13, 2021 11:37

robert-zaremba assigned technicallyty Jul 29, 2021

technicallyty suggested changes Aug 6, 2021

View reviewed changes

aaronc added the S:needs architecture review To discuss on the next architecture review call to come to alignment label Aug 6, 2021

github-actions bot added the stale label Oct 9, 2021

github-actions bot removed the stale label Oct 14, 2021

michaelfig closed this Oct 14, 2021

iramiller mentioned this pull request Jan 28, 2022

Add support for event data injection into end block handlers provenance-io/provenance#626

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement EventManager().GetABCIEventHistory() #9305

feat: implement EventManager().GetABCIEventHistory() #9305

michaelfig commented May 11, 2021 •

edited

Loading

tac0turtle commented May 20, 2021 •

edited

Loading

michaelfig commented May 22, 2021

tac0turtle commented May 22, 2021

michaelfig commented May 22, 2021

github-actions bot commented Jul 8, 2021

michaelfig commented Jul 8, 2021

technicallyty left a comment

technicallyty Aug 6, 2021

technicallyty Aug 6, 2021

technicallyty Aug 6, 2021

aaronc commented Aug 6, 2021

michaelfig commented Aug 24, 2021

github-actions bot commented Oct 9, 2021

tac0turtle commented Oct 13, 2021

robert-zaremba commented Jan 24, 2022

michaelfig commented Jan 24, 2022

robert-zaremba commented Jan 28, 2022

feat: implement EventManager().GetABCIEventHistory() #9305

feat: implement EventManager().GetABCIEventHistory() #9305

Conversation

michaelfig commented May 11, 2021 • edited Loading

Description

tac0turtle commented May 20, 2021 • edited Loading

michaelfig commented May 22, 2021

tac0turtle commented May 22, 2021

michaelfig commented May 22, 2021

github-actions bot commented Jul 8, 2021

michaelfig commented Jul 8, 2021

technicallyty left a comment

Choose a reason for hiding this comment

technicallyty Aug 6, 2021

Choose a reason for hiding this comment

technicallyty Aug 6, 2021

Choose a reason for hiding this comment

technicallyty Aug 6, 2021

Choose a reason for hiding this comment

aaronc commented Aug 6, 2021

michaelfig commented Aug 24, 2021

github-actions bot commented Oct 9, 2021

tac0turtle commented Oct 13, 2021

robert-zaremba commented Jan 24, 2022

michaelfig commented Jan 24, 2022

robert-zaremba commented Jan 28, 2022

michaelfig commented May 11, 2021 •

edited

Loading

tac0turtle commented May 20, 2021 •

edited

Loading