fix: propagate msg events correctly in x/gov

[damiannolan]

Description

This PR removes the discarding of sdk.Results in x/gov and aggregates events to be propagated to the current context upon successful execution of all gov proposal msgs

Closes: #13389

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[damiannolan]

Taking a quick look at the test failure and trying to reason about it. I'm assuming the additional event (4 instead of 3) is because a new event manager isn't created upon runMsgs where the handler is invoked.

Is it safe to just update this to expect 4 rather than 3?

[damiannolan]

Considering the e2e and unit test failures here and some discussion with @colin-axner I'm not entirely convinced this is the right fix. I'd be happy to just not discard the sdk.Result in x/gov's EndBlocker and aggregate the events there.

Would appreciate some input. cc. @alexanderbez

[alexanderbez]

woaa didn't realize this was in baseapp/msg_service_router.go -- we need the ctx = ctx.WithEventManager(sdk.NewEventManager()) call here, that's how we aggregate events per message.

[alexanderbez]

I'd be happy to just not discard the sdk.Result in x/gov's EndBlocker and aggregate the events there.

Can you describe this a bit more?

[damiannolan]

I'd be happy to just not discard the sdk.Result in x/gov's EndBlocker and aggregate the events there.

Can you describe this a bit more?

Aha! Yeah.. I had a funny feeling removing that line was a little sus.

Yeah, so I was thinking we could just not discard the sdk.Result here in gov's EndBlocker. And by doing this, we can then append the res.GetEvents() for each sdk.Msg which gets executed. When all msg handlers pass we can then flush all the response events to the current context.

What do you think?

[alexanderbez]

Yeah that SGTM -- I swear we had a PR for this, but maybe i'm thinking about something else. But in any case, 👍

[damiannolan]

You could be thinking of me fixing the same issue in x/group a few weeks back lmao 🤣

When I get some time I'll change this PR to do the above! Thanks for your input :)

[codecov]

Codecov Report

Merging #13728 (e108e62) into main (27f9851) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13728      +/-   ##
==========================================
- Coverage   55.43%   55.42%   -0.01%     
==========================================
  Files         646      646              
  Lines       55779    55784       +5     
==========================================
- Hits        30922    30921       -1     
- Misses      22341    22345       +4     
- Partials     2516     2518       +2     

Impacted Files	Coverage Δ
x/gov/abci.go	90.29% <100.00%> (+0.49%)	⬆️
x/staking/simulation/operations.go	74.54% <0.00%> (-1.38%)	⬇️

[julienrbrt]

Can we add a changelog as well 🙏

[alexanderbez]

Why do we need to declare idx and msg?

[damiannolan]

Looks like all they're used for is building a log msg below in the case of failure:
logMsg = fmt.Sprintf("passed, but msg %d (%s) failed on execution: %s", idx, sdk.MsgTypeURL(msg), err)

If they weren't declared they would only be available inside the scope of the for.

[damiannolan]

A defensive nil check on the handler here would probably be useful too.

What if I tried to exec an osmosis MsgSwap or something on the hub. I'm assuming it should fail before ever getting here, but the code looks like it relies on the conversion to Any as an unregistered type URL to prevent this.
I don't see any harm in writing this as:

if handler := keeper.Router().Handler(msg); handler != nil {
    res, err := handler(...)
...
}