feat: make authz MsgExec emit events from all executed msgs #9522

technicallyty · 2021-06-15T21:03:38Z

Description

Closes: #9501

This PR makes MsgExec emit all events from each executed message. Adds a test to check for the additional events.

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...

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)

codecov · 2021-06-15T21:49:22Z

Codecov Report

Merging #9522 (e8748ad) into master (0027111) will increase coverage by 25.12%.
The diff coverage is 62.97%.

@@             Coverage Diff             @@
##           master    #9522       +/-   ##
===========================================
+ Coverage   35.48%   60.60%   +25.12%     
===========================================
  Files         332      589      +257     
  Lines       32620    37236     +4616     
===========================================
+ Hits        11575    22567    +10992     
+ Misses      19819    12726     -7093     
- Partials     1226     1943      +717

Impacted Files	Coverage Δ
client/keys/types.go	`100.00% <ø> (+100.00%)`	⬆️
client/keys/utils.go	`42.85% <ø> (+2.50%)`	⬆️
client/query.go	`16.98% <ø> (ø)`
client/rpc/block.go	`10.00% <ø> (ø)`
client/rpc/routes.go	`100.00% <ø> (ø)`
client/rpc/status.go	`47.72% <ø> (ø)`
client/rpc/validators.go	`0.00% <ø> (ø)`
client/test_helpers.go	`0.00% <ø> (ø)`
client/tx/factory.go	`28.20% <ø> (ø)`
client/tx/legacy.go	`68.42% <ø> (ø)`
... and 700 more

yun-yeo · 2021-06-16T09:03:14Z

Can we also emit general event attributes like sender & module from message event type??

other module messages are emitting

{"type":"message","attributes":[{"key":"action","value":"/cosmos.bank.v1beta1.MsgSend"},{"key":"sender","value":"terra1rk6tvacasnnyssfnn00zl7wz43pjnpn7vayqv6"},{"key":"module","value":"bank"}]}

but authz only is emitting

{"type":"message","attributes":[{"key":"action","value":"/cosmos.authz.v1beta1.MsgExec"}]}

technicallyty · 2021-06-16T15:36:59Z

Can we also emit general event attributes like sender & module from message event type??

other module messages are emitting
{"type":"message","attributes":[{"key":"action","value":"/cosmos.bank.v1beta1.MsgSend"},{"key":"sender","value":"terra1rk6tvacasnnyssfnn00zl7wz43pjnpn7vayqv6"},{"key":"module","value":"bank"}]}

Currently I have the following being emitted after a MsgExec using bank's MsgSend on my branch:

type: coin_received
attr: [receiver: cosmos139h7762ln36evfglx5fu5mreqylh4wmj3qj47k amount: 10stake]

type: coin_spent
attr: [spender: cosmos1zqupj40s8l79qxrgl9ygknxgmt5ynjlvhuplz8 amount: 10stake]

type: message
attr: [action: /cosmos.authz.v1beta1.MsgExec sender: cosmos1zqupj40s8l79qxrgl9ygknxgmt5ynjlvhuplz8 module: bank]

type: transfer
attr: [recipient: cosmos139h7762ln36evfglx5fu5mreqylh4wmj3qj47k sender: cosmos1zqupj40s8l79qxrgl9ygknxgmt5ynjlvhuplz8 amount: 10stake]

Does this cover your needs for the issue? Let me know if there is more and I'll look into it further @YunSuk-Yeo

robert-zaremba · 2021-06-16T15:56:58Z

@YunSuk-Yeo the MsgExec and MsgSend are two different events (I'm using this names to refer for events in particular messages).

This task is to make sure that the MsgSend event is triggered when doing MsgExec for the x/bank.MsgSend.

yun-yeo · 2021-06-17T04:09:51Z

Yea actually updated events are enough, but just want to sure the consistency with other modules.

x/authz/keeper/keeper.go

amaury1093 · 2021-06-18T08:56:08Z

x/authz/keeper/keeper.go

+		for i := 0; i < len(events); i++ {
+			sdkEvents = append(sdkEvents, sdk.Event(events[i]))
+		}
+		ctx.EventManager().EmitEvents(sdkEvents)


PR looks good to me, but I actually don't understand why the underlying Msg's events are not emitted in the first place?

L112 we call msgResult, err = handler(ctx, msg), that that should call ctx.EventManager.EmitEvents() already.

I think it's because MsgExec runs its tx messages directly through tendermint/ABCI rather than the SDK handlers? Thats the gist i got from digging into it

Let's make sure we dig through the logic in the router and BaseApp and understand what's happening well

Checked some more and looks like this code overwrites the current event manager in context, which i assume could be a culprit here. I added a nil check before allowing that code to run (it was never nil in my tests).

Curiously though, it duplicated the attributes in the events. For example a transfer event had 6 attributes instead of 3 (the 3 were just duplicates, not different key/vals) Haven't had much luck figuring out why that happens though

If we removed that line in msg_service_router.go, and reverted all changes in this PR, does that still emit duplicates?

It seems to me that would be a better fix?

yeah when i ran my test i had commented out the event emission i added in DispatchEvents. For some messages though, not everything was simply a duplicate, for instance MsgExec's event only had duplicated the sender attribute, but nothing else.

also looking at this again, im noticing not every event has an exact duplicate - for example the transfer event has 2 different recipient addresses, however sender and amount are both exact copies.

robert-zaremba · 2021-06-22T00:21:28Z

x/auth/tx/service_test.go

@@ -565,6 +567,110 @@ func (s *IntegrationTestSuite) TestSimMultiSigTx() {
 	s.Require().Greater(res.GasInfo.GasUsed, uint64(0))
 }

+// Tests that all msg events included in an authz MsgExec tx
+// Ref: https://github.com/cosmos/cosmos-sdk/issues/9501
+func (s *IntegrationTestSuite) TestSim_MsgExecEvents() {


I think this test should be in x/authz

Tests moved to x/authz

x/auth/tx/service_test.go

amaury1093

pre-approving pending on Robert's comments on the test. Actual fix looks okay to me, though if we merge this we should also create an issue to understand #9522 (comment) better

robert-zaremba · 2021-06-22T11:31:44Z

[digression] Tests in SDK is a big mess - I know the fastest way is to find a similar patter and copy paste. So adding better example tests is important for future maintainability. Also tests with CLI are slower to execute.
CC: @aaronc

…smos-sdk into ty/9501-MsgExec_Events

robert-zaremba

Looks good. Left few suggestions.

robert-zaremba · 2021-06-22T18:43:11Z

x/authz/keeper/keeper_test.go

+	granterAddr := addrs[0]
+	granteeAddr := addrs[1]
+	recipientAddr := addrs[2]
+	s.Require().NoError(simapp.FundAccount(app.BankKeeper, s.ctx, granterAddr, sdk.NewCoins(sdk.NewInt64Coin("steak", 10000))))


👍 , thanks for using FundAccount

x/authz/keeper/keeper_test.go

technicallyty added 4 commits June 14, 2021 16:16

emit all events from msg dispatch

ed2efb4

Merge branch 'master' into ty/9501-MsgExec_Events

551c2e2

include a test for included events

dc4c3a5

use s.Require()

16950d9

technicallyty requested review from aaronc and alexanderbez as code owners June 15, 2021 21:03

github-actions bot added C:x/auth C:x/authz labels Jun 15, 2021

technicallyty requested review from robert-zaremba and ryanchristo June 15, 2021 21:03

order

b0601b4

blushi assigned cyberbono3 and atheeshp Jun 17, 2021

technicallyty mentioned this pull request Jun 17, 2021

fix: collect all responses from authz/MsgExec #9538

Merged

19 tasks

aaronc reviewed Jun 17, 2021

View reviewed changes

x/authz/keeper/keeper.go Outdated Show resolved Hide resolved

move into loop

e61cbba

amaury1093 mentioned this pull request Jun 18, 2021

ci: add pr task completed checker #9528

Closed

8 tasks

amaury1093 reviewed Jun 18, 2021

View reviewed changes

technicallyty and others added 4 commits June 21, 2021 08:11

Merge branch 'master' into ty/9501-MsgExec_Events

aad689f

cleanup

8c6c1fa

cleanup test

ab18843

Merge branch 'master' into ty/9501-MsgExec_Events

e1a4aeb

robert-zaremba reviewed Jun 22, 2021

View reviewed changes

amaury1093 mentioned this pull request Jun 22, 2021

Manual Testing for v0.43 regen-network/regen-ledger#352

Closed

56 tasks

amaury1093 approved these changes Jun 22, 2021

View reviewed changes

technicallyty added 3 commits June 22, 2021 08:25

Merge branch 'master' into ty/9501-MsgExec_Events

8e66ffc

move test to keeper_test

48c3d3f

Merge branch 'ty/9501-MsgExec_Events' of https://github.com/cosmos/co…

bfb7c9c

…smos-sdk into ty/9501-MsgExec_Events

github-actions bot removed the C:x/auth label Jun 22, 2021

robert-zaremba self-assigned this Jun 22, 2021

robert-zaremba approved these changes Jun 22, 2021

View reviewed changes

technicallyty and others added 2 commits June 22, 2021 11:57

cleanup tests

cb1080e

Merge branch 'master' into ty/9501-MsgExec_Events

e8748ad

technicallyty added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 22, 2021

mergify bot merged commit 899b097 into master Jun 22, 2021

mergify bot deleted the ty/9501-MsgExec_Events branch June 22, 2021 19:30

iramiller mentioned this pull request Jul 19, 2021

Release v1.7.0 testing/verification provenance-io/provenance#379

Closed

38 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make authz MsgExec emit events from all executed msgs #9522

feat: make authz MsgExec emit events from all executed msgs #9522

technicallyty commented Jun 15, 2021

codecov bot commented Jun 15, 2021 •

edited

Loading

yun-yeo commented Jun 16, 2021 •

edited

Loading

technicallyty commented Jun 16, 2021 •

edited

Loading

robert-zaremba commented Jun 16, 2021

yun-yeo commented Jun 17, 2021

amaury1093 Jun 18, 2021

technicallyty Jun 18, 2021

aaronc Jun 18, 2021

technicallyty Jun 18, 2021 •

edited

Loading

amaury1093 Jun 21, 2021 •

edited

Loading

technicallyty Jun 21, 2021 •

edited

Loading

robert-zaremba Jun 22, 2021

technicallyty Jun 22, 2021

amaury1093 left a comment •

edited

Loading

robert-zaremba commented Jun 22, 2021

robert-zaremba left a comment

robert-zaremba Jun 22, 2021

feat: make authz MsgExec emit events from all executed msgs #9522

feat: make authz MsgExec emit events from all executed msgs #9522

Conversation

technicallyty commented Jun 15, 2021

Description

Author Checklist

Reviewers Checklist

codecov bot commented Jun 15, 2021 • edited Loading

Codecov Report

yun-yeo commented Jun 16, 2021 • edited Loading

technicallyty commented Jun 16, 2021 • edited Loading

robert-zaremba commented Jun 16, 2021

yun-yeo commented Jun 17, 2021

amaury1093 Jun 18, 2021

Choose a reason for hiding this comment

technicallyty Jun 18, 2021

Choose a reason for hiding this comment

aaronc Jun 18, 2021

Choose a reason for hiding this comment

technicallyty Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

amaury1093 Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

technicallyty Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

robert-zaremba Jun 22, 2021

Choose a reason for hiding this comment

technicallyty Jun 22, 2021

Choose a reason for hiding this comment

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

robert-zaremba commented Jun 22, 2021

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba Jun 22, 2021

Choose a reason for hiding this comment

codecov bot commented Jun 15, 2021 •

edited

Loading

yun-yeo commented Jun 16, 2021 •

edited

Loading

technicallyty commented Jun 16, 2021 •

edited

Loading

technicallyty Jun 18, 2021 •

edited

Loading

amaury1093 Jun 21, 2021 •

edited

Loading

technicallyty Jun 21, 2021 •

edited

Loading

amaury1093 left a comment •

edited

Loading