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

feat: emit cached context events (backport #13063) #13702

Merged
merged 6 commits into from Oct 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -50,6 +50,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/auth/tx) [#12474](https://github.com/cosmos/cosmos-sdk/pull/12474) Remove condition in GetTxsEvent that disallowed multiple equal signs, which would break event queries with base64 strings (i.e. query by signature).
* (store) [#13530](https://github.com/cosmos/cosmos-sdk/pull/13530) Fix app-hash mismatch if upgrade migration commit is interrupted.

## API Breaking Changes

* (context) [#13063](https://github.com/cosmos/cosmos-sdk/pull/13063) Update `Context#CacheContext` to automatically emit all events on the parent context's `EventManager`.

## [v0.46.3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.46.3) - 2022-10-20

ATTENTION:
Expand Down
11 changes: 9 additions & 2 deletions types/context.go
Expand Up @@ -268,11 +268,18 @@ func (c Context) TransientStore(key storetypes.StoreKey) KVStore {

// CacheContext returns a new Context with the multi-store cached and a new
// EventManager. The cached context is written to the context when writeCache
// is called.
// is called. Note, events are automatically emitted on the parent context's
// EventManager when the caller executes the write.
func (c Context) CacheContext() (cc Context, writeCache func()) {
cms := c.MultiStore().CacheMultiStore()
cc = c.WithMultiStore(cms).WithEventManager(NewEventManager())
return cc, cms.Write

writeCache = func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the breaking part, should we spin it out into its own function and document it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will push the API breakage to 0.47. Do you think we can instead keep it API breaking and create a temporary function that keeps the old behavior? (as I have pushed)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this is API breaking personally?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is a behavior change. If we are fine with that, let me revert the addition and merge as it :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. This isn't an API break (changelog states that it is). It does introduce new or additive behavior, so I dont think its breaking. But I wont object to this PR either 👍

c.EventManager().EmitEvents(cc.EventManager().Events())
cms.Write()
}

return cc, writeCache
}

var _ context.Context = Context{}
Expand Down
5 changes: 5 additions & 0 deletions types/context_test.go
Expand Up @@ -42,13 +42,18 @@ func (s *contextTestSuite) TestCacheContext() {
s.Require().Equal(v1, cstore.Get(k1))
s.Require().Nil(cstore.Get(k2))

// emit some events
cctx.EventManager().EmitEvent(types.NewEvent("foo", types.NewAttribute("key", "value")))
cctx.EventManager().EmitEvent(types.NewEvent("bar", types.NewAttribute("key", "value")))

cstore.Set(k2, v2)
s.Require().Equal(v2, cstore.Get(k2))
s.Require().Nil(store.Get(k2))

write()

s.Require().Equal(v2, store.Get(k2))
s.Require().Len(ctx.EventManager().Events(), 2)
}

func (s *contextTestSuite) TestLogContext() {
Expand Down
6 changes: 0 additions & 6 deletions x/gov/abci.go
Expand Up @@ -84,12 +84,6 @@ func EndBlocker(ctx sdk.Context, keeper keeper.Keeper) {
tagValue = types.AttributeValueProposalPassed
logMsg = "passed"

// The cached context is created with a new EventManager. However, since
// the proposal handler execution was successful, we want to track/keep
// any events emitted, so we re-emit to "merge" the events into the
// original Context's EventManager.
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())

// write state to the underlying multi-store
writeCache()
} else {
Expand Down
8 changes: 1 addition & 7 deletions x/group/keeper/msg_server.go
Expand Up @@ -754,19 +754,13 @@ func (k Keeper) Exec(goCtx context.Context, req *group.MsgExec) (*group.MsgExecR
return nil, err
}

results, err := k.doExecuteMsgs(cacheCtx, k.router, proposal, addr)
if err != nil {
if _, err := k.doExecuteMsgs(cacheCtx, k.router, proposal, addr); err != nil {
proposal.ExecutorResult = group.PROPOSAL_EXECUTOR_RESULT_FAILURE
logs = fmt.Sprintf("proposal execution failed on proposal %d, because of error %s", id, err.Error())
k.Logger(ctx).Info("proposal execution failed", "cause", err, "proposalID", id)
} else {
proposal.ExecutorResult = group.PROPOSAL_EXECUTOR_RESULT_SUCCESS
flush()

for _, res := range results {
// NOTE: The sdk msg handler creates a new EventManager, so events must be correctly propagated back to the current context
ctx.EventManager().EmitEvents(res.GetEvents())
}
}
}

Expand Down