x/lockup: ensure events are emitted and tests added #2391

stackman27 · 2022-08-12T22:47:16Z

Closes: #2270

What is the purpose of the change

Currently, our tests are missing assertions about events. Moreover, it seems that some of our events are missing based on
1939

The lockup module emits the following events:

There are 4 types of events that exist in lockup module:

types.TypeEvtLockTokens - "lock_tokens"
types.TypeEvtAddTokensToLock - "add_tokens_to_lock"
types.TypeEvtBeginUnlockAll - "begin_unlock_all"
types.TypeEvtBeginUnlock - "begin_unlock"

Testing and Verifying

Added events test in msg_server_test
Added emit.test to test out if events emit correct events

Documentation and Release Note

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

stackman27 · 2022-08-12T22:48:26Z

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

+}
+
+// Question: Any way we can make this private method?
+func BeginUnlockEvent(lock *types.PeriodLock) sdk.Event {


We use this Event to append here

It would be ideal to make this a private method, but i dont want to repeat code

p0mvn · 2022-08-13T01:08:30Z

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

+	)
+}
+
+// EmitLockToken returns a new event when user lock tokens.


Suggested change

// EmitLockToken returns a new event when user lock tokens.

// EmitExtendLockToken returns a new event when user locks tokens.

p0mvn · 2022-08-13T01:14:05Z

x/lockup/keeper/msg_server.go

-				sdk.NewAttribute(types.AttributePeriodLockAmount, msg.Coins.String()),
-			),
-		})
+		events.EmitAddTokenToLock(ctx, lockID, msg.Owner, msg.Coins.String())


I think we should move this inside keeper so that if another module ends up calling AddToExistingLock, the event does not get missed

p0mvn · 2022-08-13T01:14:23Z

x/lockup/keeper/msg_server.go

-		),
-	})
-
+	events.EmitLockToken(ctx, &lock)


ditto for moving this inside the keeper's CreateLock

p0mvn · 2022-08-13T01:15:07Z

Can we also add the event info to docs (README), please?

stackman27 · 2022-08-13T01:49:10Z

Can we also add the event info to docs (README), please?

I didn't add README.md following @alexanderbez comment here

Didn't know what the final verdict was

p0mvn · 2022-08-13T01:52:24Z

Can we also add the event info to docs (README), please?

I didn't add README.md following @alexanderbez comment here

Didn't know what the final verdict was

I think Bez is right that these tend to get out of sync and are difficult to maintain. However, these docs get auto-uploaded to our docs.osmosis.zone website. Currently, there is no information about events on the docs site.

I think that this information could be useful for integrators. So, in my opinion, we should still have it

stackman27 · 2022-08-13T01:53:32Z

Can we also add the event info to docs (README), please?

I didn't add README.md following @alexanderbez comment here
Didn't know what the final verdict was

I think Bez is right that these tend to get out of sync and are difficult to maintain. However, these docs get auto-uploaded to our docs.osmosis.zone website. Currently, there is no information about events on the docs site.

I think that this information could be useful for integrators. So, in my opinion, we should still have it

Gotchu i'll definitely add it then

ValarDragon · 2022-08-13T04:52:24Z

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

@@ -0,0 +1,264 @@
+package events_test


This entire test file seems like useless boilerplate, its questionable that we even needed these different EmitEvent functions, we definitely don't need tests ensuring that the call to the emit function works correctly.

What tests are useful for is ensuring events are emitted after the actual.execution of relevant messages.

This makes sense, the entire boilerplate tests definitely seems like an overkill. I will remove this so that we can test messages that emit these events directly in msg_server_test.go

In my eyes, the goal at this level of abstraction is to test that attributes are set correctly. This is achieved by the tests here.

Then, it is possible to simply focus that a certain event kind is emitted at the keeper/message server layer. Given that we have already tested the attributes here in emit_test.go, we can reduce the boilerplate in tests for keeper and simply call for testing the fact that an event has been emitted:

osmosis/app/apptesting/events.go

Line 7 in a1dc694

func (s *KeeperTestHelper) AssertEventEmitted(ctx sdk.Context, eventTypeExpected string, numEventsExpected int) {

Otherwise, we would have to validate the attributes in keeper and message server tests which goes against the separation of concerns

ValarDragon · 2022-08-13T04:54:37Z

I don't really like this entire direction.

Every event here seems like we could derive it automatically from the message response, and these tests aren't actually helping check that we are emitting things correctly post message execution. At most extracting building an event to its own function makes sense to me. ctx.EventManager().EmitEvent(constructAddTokensToLockEvent(lock)), but again seems like short term status

The message_server_test changes are great, we should amend to test the real constructed event as wekk

ValarDragon · 2022-08-13T04:56:43Z

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

+	if ctx.EventManager() == nil {
+		return
+	}


Why are we doing all these nil guards? Seems fine to me if it panics with NPE, this is misconfiguration that should get caught in testing, not be graceful no-op

While I agree that no-op is probably not the way to go, I think it is worth having a check for nil event manager somewhere since ctx is injected into Osmosis's message server by its dependency - cosmos-sdk. I don't think we should be spending time on this right now though so it should be fine if we just remove the check

stackman27 · 2022-08-13T06:42:28Z

I don't really like this entire direction.

Every event here seems like we could derive it automatically from the message response, and these tests aren't actually helping check that we are emitting things correctly post message execution. At most extracting building an event to its own function makes sense to me. ctx.EventManager().EmitEvent(constructAddTokensToLockEvent(lock)), but again seems like short term status

The message_server_test changes are great, we should amend to test the real constructed event as wekk

We should definitely make a decision on how we should structure events + test events as we've followed this structure in x/gamm and x/superfluid EPIC 2195

@p0mvn what do you think?

stackman27 · 2022-08-14T19:49:47Z

x/lockup/keeper/msg_server_test.go

+					actualEvents = append(actualEvents, event)
+				}
+			}
+


line [413-421] will be moved to app/apptesting/events.go once i receive approach ack

stackman27 · 2022-08-14T19:50:31Z

x/lockup/keeper/msg_server_test.go

+			}
+
+			if tc.expectPass {
+				suite.Require().Equal(expectedEvents[0], actualEvents[0])


Tests the events type as well as its attributes

p0mvn · 2022-08-16T00:13:04Z

I don't really like this entire direction.

Every event here seems like we could derive it automatically from the message response, and these tests aren't actually helping check that we are emitting things correctly post message execution. At most extracting building an event to its own function makes sense to me. ctx.EventManager().EmitEvent(constructAddTokensToLockEvent(lock)), but again seems like short term status

The message_server_test changes are great, we should amend to test the real constructed event as week

Every event here seems like we could derive it automatically from the message response

How can events be derived from the message response?

I don't think it is possible in the example here.

First, some messages emit events based on conditions. For example, LockTokens emits TypeEvtAddTokensToLock if lock exists, TypeEvtLockTokens if it doesn't.

Second, these should really not be in the message server but in keeper. Other modules can call CreateLock bypassing lockup message server. As a result, if we have messages emitted here, they would be missed when CreateLock is called from another module.

At most extracting building an event to its own function makes sense to me

That's the goal of the internal/events package inside the module to standardize the event creation for consistency. I agree that duplicating Emit... function for every event is unnecessary and was planning to refactor that after these PRs are merged in. However, I think that we should keep the constructors.

again seems like short term status

That's fine because we can do incremental refactorings. I think changing the direction now when there is a relevant PR merged and 2 more are in progress, leading to inconsistent designs would be difficult to manage.

I also don't see how most of these events can be generated right now due to the reasons mentioned above.

The message_server_test changes are great, we should amend to test the real constructed event as week

I mentioned the original thinking for the emit_test.go to be used for validating attributes in this comment. Basically, if we test event construction at a lower level of abstraction, here we can only test the fact of a specific kind of event being emitted, without validating the attributes. The construction and attributes are already tested at a lower level of abstraction.

To summarize my thinking:

I still think that we should construct the events from the internal/events package and test the attributes there
- This construction pattern across modules makes it more visible and consistent where and when the events are emitted.
We should test the event construction / attributes in the events package
We can remove the duplicate Emit... functions and the event manager check
I think validating the event construction at the message server layer is useful and I support extracting this helper to be called in the message server tests. However, I think it might not be necessary given that we test the attributes/construction in the internal package
Some of these events should be moved to the keeper methods as per my earlier comments:
- x/lockup: ensure events are emitted and tests added #2391 (comment)
- x/lockup: ensure events are emitted and tests added #2391 (comment)

@ValarDragon @stackman27 please let me know what you think

p0mvn · 2022-08-29T21:33:22Z

Hi @stackman27

Just wanted to follow up here as well. Dev and I discussed this offline.

Let's move the events to keeper please but not use the internal/events logic. Please feel free to add tests like you were doing before.

We’ll not pursue the events QA further unless asked for by integrators and wait for improvements from the SDK

Sorry for having this blocked for quite some time

stackman27 · 2022-08-30T16:47:46Z

Hi @stackman27

Just wanted to follow up here as well. Dev and I discussed this offline.

Let's move the events to keeper please but not use the internal/events logic. Please feel free to add tests like you were doing before.

We’ll not pursue the events QA further unless asked for by integrators and wait for improvements from the SDK

Sorry for having this blocked for quite some time

Gotchu! Thank you for sorting this out. I will continue adding new tests

github-actions · 2022-09-18T00:02:31Z

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

stackman27 requested a review from a team August 12, 2022 22:47

github-actions bot added the C:x/lockup label Aug 12, 2022

stackman27 commented Aug 12, 2022

View reviewed changes

stackman27 force-pushed the lockup-events branch from 2c55587 to 9120a4a Compare August 12, 2022 22:49

stackman27 requested a review from p0mvn August 12, 2022 22:49

p0mvn reviewed Aug 13, 2022

View reviewed changes

ValarDragon reviewed Aug 13, 2022

View reviewed changes

github-actions bot added the C:app-wiring Changes to the app folder label Aug 14, 2022

stackman27 commented Aug 14, 2022

View reviewed changes

ValarDragon assigned p0mvn Aug 26, 2022

stackman27 added 3 commits September 2, 2022 21:28

x/lockup: ensure events are emitted and tests added

c54cdb6

modified event testing

2b8dcc5

adding tests

abaa467

stackman27 force-pushed the lockup-events branch from 94898bf to abaa467 Compare September 3, 2022 05:16

github-actions bot added the Stale label Sep 18, 2022

github-actions bot closed this Sep 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/lockup: ensure events are emitted and tests added #2391

x/lockup: ensure events are emitted and tests added #2391

stackman27 commented Aug 12, 2022

stackman27 Aug 12, 2022

p0mvn Aug 13, 2022

p0mvn Aug 13, 2022

p0mvn Aug 13, 2022

p0mvn commented Aug 13, 2022 •

edited

Loading

stackman27 commented Aug 13, 2022 •

edited

Loading

p0mvn commented Aug 13, 2022

stackman27 commented Aug 13, 2022

ValarDragon Aug 13, 2022 •

edited

Loading

stackman27 Aug 13, 2022

p0mvn Aug 15, 2022 •

edited

Loading

ValarDragon commented Aug 13, 2022 •

edited

Loading

ValarDragon Aug 13, 2022

p0mvn Aug 15, 2022

stackman27 commented Aug 13, 2022

stackman27 Aug 14, 2022

stackman27 Aug 14, 2022

p0mvn commented Aug 16, 2022 •

edited

Loading

p0mvn commented Aug 29, 2022 •

edited

Loading

stackman27 commented Aug 30, 2022

github-actions bot commented Sep 18, 2022

	// EmitLockToken returns a new event when user lock tokens.
	// EmitExtendLockToken returns a new event when user locks tokens.

x/lockup: ensure events are emitted and tests added #2391

x/lockup: ensure events are emitted and tests added #2391

Conversation

stackman27 commented Aug 12, 2022

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

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

p0mvn commented Aug 13, 2022 • edited Loading

stackman27 commented Aug 13, 2022 • edited Loading

p0mvn commented Aug 13, 2022

stackman27 commented Aug 13, 2022

ValarDragon Aug 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

ValarDragon commented Aug 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 commented Aug 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Aug 16, 2022 • edited Loading

p0mvn commented Aug 29, 2022 • edited Loading

stackman27 commented Aug 30, 2022

github-actions bot commented Sep 18, 2022

p0mvn commented Aug 13, 2022 •

edited

Loading

stackman27 commented Aug 13, 2022 •

edited

Loading

ValarDragon Aug 13, 2022 •

edited

Loading

p0mvn Aug 15, 2022 •

edited

Loading

ValarDragon commented Aug 13, 2022 •

edited

Loading

p0mvn commented Aug 16, 2022 •

edited

Loading

p0mvn commented Aug 29, 2022 •

edited

Loading