x/superfluid: ensure events are emitted and tests added #2255

hieuvubk · 2022-07-29T12:40:02Z

Closes: #2194

What is the purpose of the change

Brief Changelog

Refactor and unit test superfluid module events
Write test to make sure events emitted in right place
Update readme

Testing and Verifying

This change added tests and can be verified as follows:

Internal events unit tests
msg server tests

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

merge main

p0mvn

LGTM.

I verified that all attributes are preserved and the locations of where events are emitted are not changed.

Phenomenal work on this @hieuvubk ! and thank you for documenting it all!

x/superfluid/README.md

x/superfluid/keeper/gov/suite_test.go

x/superfluid/keeper/internal/events/emit.go

p0mvn · 2022-07-29T15:45:39Z

x/superfluid/keeper/msg_server.go

-			sdk.NewAttribute(types.AttributeLockId, fmt.Sprintf("%d", msg.LockId)),
-		))
+	if err == nil {
+		events.EmitSuperfluidUnbondLockEvent(ctx, msg.LockId)
 	}
 	return &types.MsgSuperfluidUnbondLockResponse{}, err


q: should we also be emitting thesdk.EventTypeMessage event at the end of every message similar to GAMM?

cc: @mattverse @ValarDragon @alexanderbez

Yeah we should use typed events for each message execution. We should also consider populating MsgSuperfluidUnbondLockResponse with relevant fields.

p0mvn · 2022-07-29T19:15:31Z

I would like to have one more reviewer give this a look before merging since the diff is large. Overall, the change LGTM. Also, I left a comment for follow-up work that would be good to decide on

czarcas7ic · 2022-07-29T21:16:24Z

x/superfluid/keeper/gov/gov_test.go

@@ -59,6 +59,7 @@ func (suite *KeeperTestSuite) TestHandleSetSuperfluidAssetsProposal() {
 		assets         []types.SuperfluidAsset
 		expectedAssets []types.SuperfluidAsset
 		expectErr      bool
+		expectedEvent  string
 	}
 	testCases := []struct {


Are we able to / should we break down this actions struct by item? Maybe its not a big deal but just seeing a lump of five items together doesn't make the table driven test super easy to figure out what is getting tested.

Good idea, naming fields is preferable for readability IMO

x/superfluid/keeper/internal/events/emit.go

x/superfluid/keeper/internal/events/emit_test.go

x/superfluid/keeper/msg_server_test.go

stackman27 · 2022-08-02T06:51:24Z

x/superfluid/keeper/internal/events/emit.go

+		types.TypeEvtSetSuperfluidAsset,
+		sdk.NewAttribute(types.AttributeDenom, denom),
+		sdk.NewAttribute(types.AttributeSuperfluidAssetType, assetType.String()),
+	)


Since we are going over all the module events, what are the thoughts on using EmitTypedEvent instead of EmitEvents

It'll definitely reduce the code size and gives a better code clarity. For instance the above code could be rewritten with an event.proto file as;

ctx.EventManager().EmitTypedEvent(&superfluid.AssetEvent{ denom: denom, assetType: assetType.String(), })

ps: EmitEvents is labelled deprecated too

cc @alexanderbez @ValarDragon @p0mvn

Yes, I think that's a great idea. However, I'm guessing we would make the EmitEvent function reusable across modules instead of defining this on the event manager. What do you think?

Also, I think we would have to make the event constructors exported.

For example, newSetSuperfluidAssetEvent -> NewSetSuperfluidAssetEvent

To have:

EmitEvent(NewSuperfluidAssetEvent(...))

Just to avoid any confusion - we should pursue this separately (not in this PR)

Are we reusing events? I would only use constructors and separate out the event logic if we use the same ones in multiple places, otherwise it would be overkill.

We are in 0 rush to switch to typed events.

I also agree that constructors seem overkill unless reuse

alexanderbez · 2022-08-03T16:00:34Z

x/superfluid/keeper/internal/events/emit.go

+}
+
+func EmitSuperfluidUnbondLockEvent(ctx sdk.Context, lockId uint64) {
+	if ctx.EventManager() == nil {


EventManager should never be nil, but I guess it can't hurt. Just so many LOC :-/

Ahh thank you. If so, I'll remove them.

alexanderbez · 2022-08-03T16:00:56Z

x/superfluid/README.md

@@ -455,7 +455,88 @@ Disable multiple assets from being used for superfluid staking.

 ## Events


I love the motivation for this, but I can see this going out of date and being difficult to maintain.

I think we might still want to have it since these docs get auto-uploaded to the docs website. Currently, there is no info about events but it could be useful for integrators. WDYT?

merge main

stackman27 · 2022-08-10T05:47:34Z

x/superfluid/keeper/msg_server_test.go

+
+// TestMsgSuperfluidDelegate_Event tests that events are correctly emitted
+// when calling SuperfluidDelegate.
+func (suite *KeeperTestSuite) TestMsgSuperfluidDelegate_Event() {


I noticed that events tests are very identical to their actual tests and there is repetitive code in tests like (TestMsgSuperfluidDelegate and TestMsgSuperfluidDelegate_Event)

is there any way we can just use one?

stackman27 · 2022-08-10T05:49:56Z

x/superfluid/keeper/msg_server_test.go

+	}
+}
+
+func assertEventEmitted(suite *KeeperTestSuite, ctx sdk.Context, eventTypeExpected string, numEventsExpected int) {


this function has been defined in app/apptesting/events.go pretty sure we can just use that

Thanks for reminding

stackman27 · 2022-08-10T06:08:35Z

x/superfluid/keeper/msg_server_test.go

+
+		if test.expectPass {
+			suite.Require().NoError(err)
+			assertEventEmitted(suite, suite.Ctx, types.TypeEvtSuperfluidDelegate, 1)


why do you need to pass Suite and Suite.Ctx? You can just get the context if you pass Suite

x/superfluid/keeper/internal/events/emit.go

stackman27 · 2022-08-12T06:55:20Z

x/superfluid/keeper/internal/events/emit_test.go

+	"github.com/osmosis-labs/osmosis/v10/x/superfluid/types"
+)
+
+type SuperfluidEventsTestSuite struct {


Just wondering, what was the motivation behind create a new test suite? Can't we just use the KeeperTestSuite?

cc @p0mvn

stackman27 · 2022-08-12T06:55:21Z

x/superfluid/keeper/internal/events/emit_test.go

+	addressString = "addr1---------------"
+	testDenomA    = "denoma"
+	testDenomB    = "denomb"
+	testDenomC    = "denomc"


this seems to be unused

stackman27 · 2022-08-12T06:55:22Z

x/superfluid/keeper/internal/events/emit_test.go

+	testDenomA    = "denoma"
+	testDenomB    = "denomb"
+	testDenomC    = "denomc"
+	testDenomD    = "denomd"


this seems to be unused

Co-authored-by: Sishir Giri <[email protected]>

merge main

ValarDragon

LGTM

I do have the same general point of I think this should be managed by the framework / we shouldn't do more of this, but I think this is good to merge in!

Thanks for the fix, and sorry this got blocked for so long

hieuvubk added 8 commits July 28, 2022 15:45

Merge pull request #51 from osmosis-labs/main

963b0f4

merge main

add emit event & test

9abb176

add test for emit event

cd82fe7

format

c0dd4b1

add event test for hook

20cba08

add event test for unpool

3dbe40c

refactor

e8b4fca

update readme

f357e10

hieuvubk requested a review from a team July 29, 2022 12:40

github-actions bot added C:docs Improvements or additions to documentation C:x/superfluid labels Jul 29, 2022

hieuvubk added 2 commits July 29, 2022 20:03

format codes

41f8b78

remove unuse

8390a5a

p0mvn approved these changes Jul 29, 2022

View reviewed changes

fix readme

e72df0d

czarcas7ic reviewed Jul 29, 2022

View reviewed changes

restruct test

eddaf55

hieuvubk requested a review from czarcas7ic August 1, 2022 02:40

stackman27 reviewed Aug 2, 2022

View reviewed changes

alexanderbez reviewed Aug 3, 2022

View reviewed changes

Merge pull request #53 from osmosis-labs/main

1e2b423

merge main

github-actions bot added C:app-wiring Changes to the app folder C:CLI C:simulator Edits simulator or simulations C:x/gamm Changes, features and bugs related to the gamm module. C:x/incentives C:x/lockup C:x/mint C:x/tokenfactory labels Aug 4, 2022

github-actions bot removed C:simulator Edits simulator or simulations C:x/lockup T:CI T:build C:CLI labels Aug 4, 2022

stackman27 reviewed Aug 10, 2022

View reviewed changes

hieuvubk added 4 commits August 11, 2022 12:57

duplicate test

58fbe27

use suite.AssertEventEmitted

5178a31

hooks test

d7102b8

format

f119784

hieuvubk requested a review from stackman27 August 11, 2022 06:03

usegov tests use apptesting

9506eb1

hieuvubk requested review from alexanderbez and ValarDragon August 11, 2022 12:51

stackman27 reviewed Aug 12, 2022

View reviewed changes

x/superfluid/keeper/internal/events/emit.go Outdated Show resolved Hide resolved

stackman27 reviewed Aug 12, 2022

View reviewed changes

hieuvubk and others added 6 commits August 12, 2022 14:00

Update x/superfluid/keeper/internal/events/emit.go

f42e205

Co-authored-by: Sishir Giri <[email protected]>

remove unused variables

bc9cbf5

merge main

9c79c43

v11

d3dd488

format

7ec606b

resolve conflict

d808e28

hieuvubk requested a review from stackman27 August 27, 2022 08:18

Merge pull request #62 from osmosis-labs/main

ac87b0f

merge main

ValarDragon approved these changes Sep 3, 2022

View reviewed changes

ValarDragon merged commit 0212dee into osmosis-labs:main Sep 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/superfluid: ensure events are emitted and tests added #2255

x/superfluid: ensure events are emitted and tests added #2255

hieuvubk commented Jul 29, 2022

p0mvn left a comment

p0mvn Jul 29, 2022

alexanderbez Aug 3, 2022

p0mvn commented Jul 29, 2022 •

edited

Loading

czarcas7ic Jul 29, 2022

p0mvn Jul 29, 2022

stackman27 Aug 2, 2022 •

edited

Loading

p0mvn Aug 3, 2022 •

edited

Loading

alexanderbez Aug 3, 2022

ValarDragon Aug 4, 2022 •

edited

Loading

alexanderbez Aug 3, 2022

hieuvubk Aug 4, 2022

alexanderbez Aug 3, 2022

p0mvn Aug 13, 2022

stackman27 Aug 10, 2022 •

edited

Loading

stackman27 Aug 10, 2022

hieuvubk Aug 11, 2022

stackman27 Aug 10, 2022

stackman27 Aug 12, 2022

stackman27 Aug 12, 2022

stackman27 Aug 12, 2022

hieuvubk Aug 12, 2022

ValarDragon left a comment

		@@ -455,7 +455,88 @@ Disable multiple assets from being used for superfluid staking.

		## Events

x/superfluid: ensure events are emitted and tests added #2255

x/superfluid: ensure events are emitted and tests added #2255

Conversation

hieuvubk commented Jul 29, 2022

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

p0mvn Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

p0mvn commented Jul 29, 2022 •

edited

Loading

stackman27 Aug 2, 2022 •

edited

Loading

p0mvn Aug 3, 2022 •

edited

Loading

ValarDragon Aug 4, 2022 •

edited

Loading

stackman27 Aug 10, 2022 •

edited

Loading