Protorev: Backrun event emission #4878

NotJeremyLiu · 2023-04-10T16:01:28Z

What is the purpose of the change

Adds protorev-specific event emission when the module successfully backruns to make it easier to index for tracking, research, and visualization purposes.

Brief Changelog

Emits a protorev_backrun event upon successful backrunning of a transaction. Emits details about 1) user denoms, 2) pool points remaining, 3) profit and denom, 4) amount in and out to execute trade, and 5) tx_hash

Testing and Verifying

(Please pick one of the following options)

All tests current tests pass after changing input params necessary for event
Added unit test for the added event

Documentation and Release Note

Added event section to protorev.md and added the protorev_backrun event type and attrs.

davidterpay

Update LGTM, but would move the creation and event emission into the same place.

x/protorev/types/events.go

x/protorev/keeper/emit.go

x/protorev/keeper/posthandler.go

x/protorev/keeper/emit.go

ValarDragon · 2023-04-11T17:41:35Z

We discussed on call that we are concerned that the keeper call here may change gas. Resolution on call I believe was to:

merge this PR
on v15 backport be careful to ensure that we delete the getter and usage of that getter in the event

NotJeremyLiu · 2023-04-11T18:05:22Z

We discussed on call that we are concerned that the keeper call here may change gas. Resolution on call I believe was to:

merge this PR

on v15 backport be careful to ensure that we delete the getter and usage of that getter in the event

I'm going to spend a bit of time today refactoring to not include an extra kvstore read, but to see if we can still output the block pool points. Tbh, this is the main reason why we want this event as it will allow us to know how much room we still have to per block to backrun more or less.

NotJeremyLiu · 2023-04-11T21:30:11Z

Need to add tests to verify, but this commit intends to allow us to emit blockPoolPointsRemaining without doing an extra kvstore read: 482cbf6

NotJeremyLiu · 2023-04-17T18:06:01Z

Test added, ready for review

p0mvn · 2023-04-18T00:29:48Z

x/protorev/keeper/emit.go

+// EmitBackrunEvent updates and emits a backrunEvent
+func EmitBackrunEvent(ctx sdk.Context, pool SwapToBackrun, inputCoin sdk.Coin, profit, tokenOutAmount sdk.Int, remainingTxPoolPoints, remainingBlockPoolPoints uint64) error {
+	// Get tx hash
+	txHash := strings.ToUpper(hex.EncodeToString(tmhash.Sum(ctx.TxBytes())))
+	// Update the backrun event and add it to the context
+	backrunEvent := sdk.NewEvent(
+		types.TypeEvtBackrun,
+		sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
+		sdk.NewAttribute(types.AttributeKeyTxHash, txHash),
+		sdk.NewAttribute(types.AttributeKeyUserPoolId, strconv.FormatUint(pool.PoolId, 10)),
+		sdk.NewAttribute(types.AttributeKeyUserDenomIn, pool.TokenInDenom),
+		sdk.NewAttribute(types.AttributeKeyUserDenomOut, pool.TokenOutDenom),
+		sdk.NewAttribute(types.AttributeKeyTxPoolPointsRemaining, strconv.FormatUint(remainingTxPoolPoints, 10)),
+		sdk.NewAttribute(types.AttributeKeyBlockPoolPointsRemaining, strconv.FormatUint(remainingBlockPoolPoints, 10)),
+		sdk.NewAttribute(types.AttributeKeyProtorevProfit, profit.String()),
+		sdk.NewAttribute(types.AttributeKeyProtorevAmountIn, inputCoin.Amount.String()),
+		sdk.NewAttribute(types.AttributeKeyProtorevAmountOut, tokenOutAmount.String()),
+		sdk.NewAttribute(types.AttributeKeyProtorevArbDenom, inputCoin.Denom),
+	)
+	ctx.EventManager().EmitEvent(backrunEvent)
+
+	return nil
+}


There does not seem to be an error returned, can we remove error return?

Resolved in: f632b42

p0mvn · 2023-04-18T00:31:06Z

x/protorev/keeper/rebalance.go

+	if err != nil {
+		return err
+	}


Note that adding error checks can be state-breaking. However, it does not seem that EmitBackrunEvent actually ever returns an error so we can probably remove it

Noted, resolved in f632b42 by removing the unused error return

p0mvn · 2023-04-18T00:35:49Z

x/protorev/keeper/rebalance.go

+	if numberOfAvailablePoolPoints > maxPoolPointsPerTx {
+		return maxPoolPointsPerTx, numberOfAvailablePoolPoints, nil
 	}

-	return numberOfIterableRoutes, nil
+	return numberOfAvailablePoolPoints, numberOfAvailablePoolPoints, nil


Could you please update godoc indicating what the return values are and when max vs available are returned?

Updated in: 03f8e98

NotJeremyLiu · 2023-04-18T05:27:34Z

@p0mvn All requested changes have been done, re-review ready

…r event - Instead of reading from store to create the event (previously read to determine remaining block-level pool points remaining), use a previously read variable and passes it through the necessary functions to be able to emit it at the end

NotJeremyLiu · 2023-04-18T14:07:16Z

Rebased and added change log entry, should be ready to merge

* Add protorev backrun_event emission * add empty backrun event to pass in as param to make rebalance tests pass * Add basic validity test for protorev_backrun event * Add pool id to event, add documentation * Add tx_hash to backrun event * change pool point var naming * Consolidate create and emit backrun event functions into one * Update logic to not require an additional kvstore read to get data for event - Instead of reading from store to create the event (previously read to determine remaining block-level pool points remaining), use a previously read variable and passes it through the necessary functions to be able to emit it at the end * Rename pool points fn to be more general * Add test to verify proper event emission * add tx_hash to event documentation * Remove unused error return * Update godoc and naming for more clarity * add changelog entry (cherry picked from commit 62968cc)

* Add protorev backrun_event emission * add empty backrun event to pass in as param to make rebalance tests pass * Add basic validity test for protorev_backrun event * Add pool id to event, add documentation * Add tx_hash to backrun event * change pool point var naming * Consolidate create and emit backrun event functions into one * Update logic to not require an additional kvstore read to get data for event - Instead of reading from store to create the event (previously read to determine remaining block-level pool points remaining), use a previously read variable and passes it through the necessary functions to be able to emit it at the end * Rename pool points fn to be more general * Add test to verify proper event emission * add tx_hash to event documentation * Remove unused error return * Update godoc and naming for more clarity * add changelog entry (cherry picked from commit 62968cc) Co-authored-by: Jeremy Liu <[email protected]>

NotJeremyLiu requested a review from davidterpay April 10, 2023 16:01

NotJeremyLiu added the V:state/compatible/backport State machine compatible PR, should be backported label Apr 10, 2023

NotJeremyLiu changed the title ~~Jl/protorev event emission~~ Protorev: Backrun event emission Apr 10, 2023

NotJeremyLiu added the protorev All things related to x/protorev label Apr 10, 2023

NotJeremyLiu mentioned this pull request Apr 10, 2023

[v16]: Scoping issue #4700

Closed

github-actions bot added the C:docs Improvements or additions to documentation label Apr 10, 2023

NotJeremyLiu marked this pull request as ready for review April 10, 2023 17:07

davidterpay reviewed Apr 10, 2023

View reviewed changes

ValarDragon added the A:backport/v15.x backport patches to v15.x branch label Apr 11, 2023

NotJeremyLiu requested a review from p0mvn as a code owner April 11, 2023 20:35

NotJeremyLiu mentioned this pull request Apr 17, 2023

ProtoRev: Event / Query Patch Tracking #4942

Closed

2 tasks

p0mvn reviewed Apr 18, 2023

View reviewed changes

NotJeremyLiu requested a review from p0mvn April 18, 2023 05:27

p0mvn approved these changes Apr 18, 2023

View reviewed changes

NotJeremyLiu added 11 commits April 18, 2023 10:05

Add protorev backrun_event emission

c179f03

add empty backrun event to pass in as param to make rebalance tests pass

cd65fef

Add basic validity test for protorev_backrun event

c82afe1

Add pool id to event, add documentation

921fb45

Add tx_hash to backrun event

8b7394e

change pool point var naming

16af403

Consolidate create and emit backrun event functions into one

074da80

Rename pool points fn to be more general

2d6ba3e

Add test to verify proper event emission

8a2d636

add tx_hash to event documentation

dd4124e

NotJeremyLiu added 2 commits April 18, 2023 10:05

Remove unused error return

f05ecca

Update godoc and naming for more clarity

337269e

NotJeremyLiu force-pushed the jl/protorev-event-emission branch from 03f8e98 to 337269e Compare April 18, 2023 14:06

add changelog entry

e9dc64a

p0mvn approved these changes Apr 19, 2023

View reviewed changes

p0mvn requested review from nicolaslara and stackman27 April 19, 2023 17:46

nicolaslara approved these changes Apr 19, 2023

View reviewed changes

nicolaslara merged commit 62968cc into main Apr 19, 2023

nicolaslara deleted the jl/protorev-event-emission branch April 19, 2023 18:13

mergify bot mentioned this pull request Apr 19, 2023

Protorev: Backrun event emission (backport #4878) #4963

Merged

github-actions bot mentioned this pull request Oct 15, 2024

Broken Links Detected notional-labs/osmosis#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protorev: Backrun event emission #4878

Protorev: Backrun event emission #4878

NotJeremyLiu commented Apr 10, 2023 •

edited

Loading

davidterpay left a comment

ValarDragon commented Apr 11, 2023

NotJeremyLiu commented Apr 11, 2023

NotJeremyLiu commented Apr 11, 2023

NotJeremyLiu commented Apr 17, 2023

p0mvn Apr 18, 2023

NotJeremyLiu Apr 18, 2023

p0mvn Apr 18, 2023

NotJeremyLiu Apr 18, 2023

p0mvn Apr 18, 2023

NotJeremyLiu Apr 18, 2023

NotJeremyLiu commented Apr 18, 2023

NotJeremyLiu commented Apr 18, 2023

Protorev: Backrun event emission #4878

Protorev: Backrun event emission #4878

Conversation

NotJeremyLiu commented Apr 10, 2023 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

davidterpay left a comment

Choose a reason for hiding this comment

ValarDragon commented Apr 11, 2023

NotJeremyLiu commented Apr 11, 2023

NotJeremyLiu commented Apr 11, 2023

NotJeremyLiu commented Apr 17, 2023

p0mvn Apr 18, 2023

Choose a reason for hiding this comment

NotJeremyLiu Apr 18, 2023

Choose a reason for hiding this comment

p0mvn Apr 18, 2023

Choose a reason for hiding this comment

NotJeremyLiu Apr 18, 2023

Choose a reason for hiding this comment

p0mvn Apr 18, 2023

Choose a reason for hiding this comment

NotJeremyLiu Apr 18, 2023

Choose a reason for hiding this comment

NotJeremyLiu commented Apr 18, 2023

NotJeremyLiu commented Apr 18, 2023

NotJeremyLiu commented Apr 10, 2023 •

edited

Loading