[Application] Add missing (un)staking events #867

bryanchriswhite · 2024-10-08T12:35:16Z

Summary

Adds:

EventApplicationStake
EventApplicationUnbondingBegin
EventApplicationUnbondingEnd
EventApplicationUnbondingCanceled
EventApplicationUnbondedBelowMinStake

Issue

[Events] Add missing events #858

Type of change

Select one or more from the following:

New feature, functionality or library
Consensus breaking; add the consensus-breaking label if so. See [Infra] Automatically add the consensus-breaking label #791 for details
Bug fix
Code health or cleanup
Documentation
Other (specify)

Testing

Documentation: make docusaurus_start; only needed if you make doc changes
Unit Tests: make go_develop_and_test
LocalNet E2E Tests: make test_e2e
DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

Sanity Checklist

I have tested my changes using the available tooling
I have commented my code
I have performed a self-review of my own code; both comments & source code
I create and reference any new tickets, if applicable
I have left TODOs throughout the codebase, if applicable

…nbondingEnd

x/application/keeper/msg_server_stake_application_test.go

x/application/keeper/msg_server_stake_application.go

…application * issues/612/application/burning: (94 commits) fix: linter error chore: quick fixes [DifficultyHash] Prepare for difficulty multiplier usage (#836) [Application] Enforce minimum stake when staking (#847) Empty commit [Tokenomics] Prevent GMR to produce zero values (#866) Empty commit chore: regenerate protobufs fix: linter error Empty commit chore: review feedback improvements Empty commit Empty commit fix: linter errors fix: typo chore: review feedback improvements chore: reconcile PreGeneratedAccountIterator#MustNext() chore: add review feedback TODOs Empty commit test: simplify coin equality assertions ... # Conflicts: # x/application/keeper/msg_server_stake_application_test.go # x/application/keeper/msg_server_unstake_application.go # x/application/keeper/msg_server_unstake_application_test.go # x/application/keeper/unbond_applications.go

…bondingCanceled

proto/poktroll/application/event.proto

tests/integration/application/min_stake_test.go

x/application/keeper/msg_server_unstake_application.go

x/application/keeper/msg_server_stake_application.go

…application

bryanchriswhite · 2024-10-11T11:41:45Z

proto/poktroll/application/event.proto

+
+// EventApplicationUnbondingCanceled is emitted when an application which was unbonding
+// successfully (re-)stakes before the unbonding period has elapsed. An EventApplicationStaked
+// event will also be emmitted immediatly after this event.


Suggested change

// event will also be emmitted immediatly after this event.

// event will also be emitted immediately after this event.

bryanchriswhite · 2024-10-11T11:52:36Z

x/application/keeper/msg_server_stake_application.go

+	for _, event := range events {
+		if err = sdkCtx.EventManager().EmitTypedEvent(event); err != nil {
+			err = types.ErrAppEmitEvent.Wrapf("(%+v): %s", event, err)
+			logger.Error(err.Error())
+			return nil, status.Error(codes.Internal, err.Error())
+		}
+	}


Suggested change

for _, event := range events {

if err = sdkCtx.EventManager().EmitTypedEvent(event); err != nil {

err = types.ErrAppEmitEvent.Wrapf("(%+v): %s", event, err)

logger.Error(err.Error())

return nil, status.Error(codes.Internal, err.Error())

}

}

if err = sdkCtx.EventManager().EmitTypedEvents(events...); err != nil {

err = types.ErrAppEmitEvent.Wrapf("(%+v): %s", events, err)

logger.Error(err.Error())

return nil, status.Error(codes.Internal, err.Error())

}

bryanchriswhite · 2024-10-11T13:00:49Z

proto/poktroll/application/event.proto

+// EventApplicationUnbondingBegin is emitted when an application unstake message
+// is committed, indicating that an application has begun unbonding.
+message EventApplicationUnbondingBegin {
+  string app_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];


Add unbonding_height field for consistency with EventSupplierUnbondingBegin.

bryanchriswhite · 2024-10-11T13:00:58Z

proto/poktroll/application/event.proto

+// unbonding. The unbonding period is determined by the shared param,
+// application_unbonding_period_sessions.
+message EventApplicationUnbondingEnd {
+  string app_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];


Add unbonding_height field for consistency with EventSupplierUnbondingEnd.

bryanchriswhite · 2024-10-11T14:18:16Z

proto/poktroll/application/event.proto

+// unbonded during settlement because their post-settlement stake dropped below
+// the minimum application stake requirement.
+message EventApplicationUnbondedBelowMinStake {
+  string app_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];


Add unbonding_height and application_min_stake fields.

bryanchriswhite · 2024-10-11T14:20:58Z

proto/poktroll/application/event.proto

+// successfully (re-)stakes before the unbonding period has elapsed. An EventApplicationStaked
+// event will also be emmitted immediatly after this event.
+message EventApplicationUnbondingCanceled {
+  string app_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];


Add session_end_height field

red-0ne

An important milestone achieved! 😄

Left some comments w.r.t. the events content but looks good otherwise 👍

red-0ne · 2024-10-11T16:49:00Z

proto/poktroll/application/event.proto

 import "poktroll/application/types.proto";

+// EventApplicationStaked is emitted when an application is staked or up-staked.
+message EventApplicationStaked {


Why not just embed the whole application?

red-0ne · 2024-10-11T16:50:33Z

testutil/events/reset.go

+// such that prior event emissions are cleared.
+func ResetEventManager(ctx context.Context) context.Context {
+	return cosmostypes.UnwrapSDKContext(ctx).
+		WithEventManager(cosmostypes.NewEventManager())


red-0ne · 2024-10-11T16:52:20Z

x/tokenomics/keeper/token_logic_modules.go

@@ -283,6 +283,16 @@ func (k Keeper) ProcessTokenLogicModules(
 	// TODO_CONSIDERATION: If we support multiple native tokens, we will need to
 	// start checking the denom here.
 	if application.Stake.Amount.LT(apptypes.DefaultMinStake.Amount) {
+		sdkCtx := sdk.UnwrapSDKContext(ctx)
+		unbondedBelowMinStakeEvent := &apptypes.EventApplicationUnbondedBelowMinStake{
+			AppAddress: application.GetAddress(),


Any opinion on sending the whole application? (at least the stake amount)

red-0ne · 2024-10-11T16:53:39Z

x/application/keeper/unbond_applications.go

-		// TODO_UPNEXT(@bryanchriswhite): emit a new EventApplicationUnbondingEnd event.
+		sdkCtx := sdk.UnwrapSDKContext(ctx)
+		unbondingBeginEvent := &apptypes.EventApplicationUnbondingEnd{
+			AppAddress: application.GetAddress(),


Would be nice to see the stake being returned to the app account. IMO Adding the whole app to the event is OK.

red-0ne · 2024-10-11T16:56:14Z

x/application/keeper/msg_server_unstake_application.go

-	// TODO_UPNEXT:(@bryanchriswhite): emit a new EventApplicationUnbondingBegin event.
+	sdkCtx = sdk.UnwrapSDKContext(ctx)
+	unbondingBeginEvent := &types.EventApplicationUnbondingBegin{
+		AppAddress: foundApp.GetAddress(),


Same here, having a "snapshot" of the application state (current stake) could be useful, in conjunction with the unbonding end event having the same.

We would be able to see how much stake has been deducted in between. We probably have other means to know it though.

red-0ne · 2024-10-11T17:23:25Z

x/application/keeper/msg_server_delegate_to_gateway_test.go

 	events = sdkCtx.EventManager().Events()
-	require.Equal(t, 1, len(events))


This was appearing because the events did not get cleared right?

bryanchriswhite added 3 commits October 8, 2024 13:35

feat: add & emit EventApplicationStaked and EventApplicationUpStaked

66f0344

feat: add & emit EventApplicationUnbondingBegin and EventApplicationU…

1678dbe

…nbondingEnd

chore: regenerate protobufs

da1dc9a

bryanchriswhite added application Changes related to the Application actor on-chain On-chain business logic code health Cleans up some code labels Oct 8, 2024

bryanchriswhite added this to the Shannon Beta TestNet Launch milestone Oct 8, 2024

bryanchriswhite self-assigned this Oct 8, 2024

bryanchriswhite linked an issue Oct 8, 2024 that may be closed by this pull request

[Events] Add missing events #858

Open

5 tasks

bryanchriswhite added 2 commits October 8, 2024 14:58

fix: typo

14e6fb2

fix: return event errors

672db5d

bryanchriswhite mentioned this pull request Oct 8, 2024

[Events] Add missing events #858

Open

5 tasks

bryanchriswhite marked this pull request as ready for review October 8, 2024 13:02

bryanchriswhite requested review from Olshansk and red-0ne October 8, 2024 13:02

chore: remove EventApplicationUpStake (changed my mind)

0171a40

bryanchriswhite commented Oct 9, 2024

View reviewed changes

x/application/keeper/msg_server_stake_application_test.go Outdated Show resolved Hide resolved

bryanchriswhite commented Oct 9, 2024

View reviewed changes

x/application/keeper/msg_server_stake_application.go Outdated Show resolved Hide resolved

bryanchriswhite added 2 commits October 9, 2024 09:31

chore: add testutil/events.ResetEventManager()

cf382d6

fix: linter error

2bd52fc

bryanchriswhite force-pushed the issues/858/events/application branch from ea2ce03 to 2bd52fc Compare October 9, 2024 07:32

bryanchriswhite marked this pull request as draft October 10, 2024 11:50

bryanchriswhite changed the base branch from main to issues/612/application/burning October 10, 2024 12:07

bryanchriswhite added 3 commits October 10, 2024 14:43

chore: add EventApplicationUnbondedBelowMinStake & EventApplicationUn…

432946e

…bondingCanceled

fix: failing unit tests

ae7a5e3

Empty commit

f45b885

bryanchriswhite commented Oct 10, 2024

View reviewed changes

proto/poktroll/application/event.proto Show resolved Hide resolved

bryanchriswhite commented Oct 10, 2024

View reviewed changes

tests/integration/application/min_stake_test.go Outdated Show resolved Hide resolved

bryanchriswhite commented Oct 10, 2024

View reviewed changes

x/application/keeper/msg_server_unstake_application.go Outdated Show resolved Hide resolved

bryanchriswhite commented Oct 10, 2024

View reviewed changes

x/application/keeper/msg_server_stake_application.go Outdated Show resolved Hide resolved

bryanchriswhite added 3 commits October 11, 2024 11:59

chore: self-review improvements

e49a48c

fix: linter error

a1f1e2a

Merge branch 'issues/612/application/burning' into issues/858/events/…

7c9b195

…application

bryanchriswhite marked this pull request as ready for review October 11, 2024 10:02

bryanchriswhite commented Oct 11, 2024

View reviewed changes

red-0ne reviewed Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Application] Add missing (un)staking events #867

[Application] Add missing (un)staking events #867

bryanchriswhite commented Oct 8, 2024 •

edited

Loading

bryanchriswhite Oct 11, 2024 •

edited

Loading

bryanchriswhite Oct 11, 2024 •

edited

Loading

bryanchriswhite Oct 11, 2024

bryanchriswhite Oct 11, 2024

bryanchriswhite Oct 11, 2024

bryanchriswhite Oct 11, 2024

red-0ne left a comment

red-0ne Oct 11, 2024

red-0ne Oct 11, 2024

red-0ne Oct 11, 2024

red-0ne Oct 11, 2024

red-0ne Oct 11, 2024

red-0ne Oct 11, 2024

	// event will also be emmitted immediatly after this event.
	// event will also be emitted immediately after this event.

		events = sdkCtx.EventManager().Events()
		require.Equal(t, 1, len(events))

[Application] Add missing (un)staking events #867

Are you sure you want to change the base?

[Application] Add missing (un)staking events #867

Conversation

bryanchriswhite commented Oct 8, 2024 • edited Loading

Summary

Issue

Type of change

Testing

Sanity Checklist

bryanchriswhite Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

bryanchriswhite Oct 11, 2024 • 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

red-0ne left a 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

bryanchriswhite commented Oct 8, 2024 •

edited

Loading

bryanchriswhite Oct 11, 2024 •

edited

Loading

bryanchriswhite Oct 11, 2024 •

edited

Loading