chore(perf): protorev tracker coin array #7240

czarcas7ic · 2024-01-04T04:49:58Z

Closes: #XXX

What is the purpose of the change

This is Part 2/2 in protorev tracking perf changes.

This PR utilizes a new key store and tracks by denom key, which prevents us from having to serialize every coin whenever taker fees are tracked.

Side note: There were three proto rev trackers:

Tx fees: Is now removed
Taker Fees: Modified to coin array here
Cyclic Arb: We dont modify this logic, because it is called once at time of initialization, and then compared to the cyclic arb profits that protorev tracks and the difference is taken. It is not called on its own after every action like the taker fee and tx fee trackers

Testing and Verifying

Ran the query in E2E after the upgrade and noted that:

The count resets
The block height changes to the upgrade height as expected

czarcas7ic · 2024-01-05T20:39:35Z

tests/e2e/configurer/config/constants.go

+	PropBufferBlocks float32 = 30
+	// number of blocks used as a calculation buffer (used to set actual voting period)
+	PropBufferBlocksVotePeriod float32 = 8


We now run multiple blocks a second both pre and post upgrade. The 30 prop buffer blocks are needed to allow enough time to pass for the upgrade prop for the upgrade height to not be in the past.

czarcas7ic · 2024-01-05T20:40:21Z

x/poolmanager/protorev.go

-			Amount: newTakerFeeForStakersCoins,
+// GetTakerFeeTrackerForStakers returns the taker fee for stakers tracker for all denoms that has been
+// collected since the accounting height.
+func (k Keeper) GetTakerFeeTrackerForStakers(ctx sdk.Context) []sdk.Coin {


The Getters, Getters by Denom, and Update logic follows the same pattern that protorev uses for tracking profits by denom.

czarcas7ic · 2024-01-05T20:41:07Z

x/poolmanager/protorev_test.go

 )

-func (s *KeeperTestSuite) TestGetSetTakerFeeTrackerForStakersAndCommunityPool() {
+func (s *KeeperTestSuite) TestGetTakerFeeTrackerForStakersAndCommunityPool() {


We remove Setter logic in this test. We now only have Get and Update, where update only increases and does not overwrite.

czarcas7ic · 2024-01-05T20:43:02Z

x/poolmanager/taker_fee.go

@@ -144,33 +144,39 @@ func (k Keeper) chargeTakerFee(ctx sdk.Context, tokenIn sdk.Coin, tokenOutDenom
 	takerFeeAmtRemaining := takerFeeCoin.Amount
 	if takerFeeCoin.Denom == defaultTakerFeeDenom {
 		// Community Pool:
-		if poolManagerParams.TakerFeeParams.OsmoTakerFeeDistribution.CommunityPool.GT(osmomath.ZeroDec()) {
+		if poolManagerParams.TakerFeeParams.OsmoTakerFeeDistribution.CommunityPool.GT(osmomath.ZeroDec()) && takerFeeAmtRemaining.GT(osmomath.ZeroInt()) {


Notice I added && takerFeeAmtRemaining.GT(osmomath.ZeroInt())

I noticed in some local tests that its possible for there to be zero taker fee amt when taker fee for denom is 0, despite the fee distribution percent being non zero. This doesn't really do anything bad, but does include a zero entry to the tracker. Again, not likely on prod and even if it did happen its a non issue, but was just not clean.

czarcas7ic · 2024-01-05T20:43:49Z

x/poolmanager/taker_fee.go

+			if err != nil {
+				ctx.Logger().Error("Error updating taker fee tracker for community pool by denom", "error", err)
+			}


Notice for all the logs here, I just print the error to logs. My mindset is, a bug in the tracker logic should not prevent all swap actions from happening.

czarcas7ic · 2024-01-05T20:44:25Z

x/poolmanager/types/keys.go

+
+	// KeyTakerFeeStakersProtoRevArray defines key to store the taker fee for stakers tracker coin array.
+	KeyTakerFeeStakersProtoRevArray = []byte{0x08}
+
+	// KeyTakerFeeCommunityPoolProtoRevArray defines key to store the taker fee for community pool tracker coin array.
+	KeyTakerFeeCommunityPoolProtoRevArray = []byte{0x09}


I could have probably used the old key, but decided on using a new one to just prevent any possible unforeseen bugs.

…ified dependencies locally

czarcas7ic · 2024-01-05T21:14:08Z

x/poolmanager/msg_server_test.go

+			poolManagerParams := s.App.PoolManagerKeeper.GetParams(ctx)
+			poolManagerParams.TakerFeeParams.DefaultTakerFee = sdk.MustNewDecFromStr("0.01")
+			s.App.PoolManagerKeeper.SetParams(ctx, poolManagerParams)
+


These needed to be added due to the && that was added to the taker fee logic. This test before was going through the loop with a taker fee of 0%, so now if it isn't set we just skip this logic.

ValarDragon · 2024-01-07T18:39:05Z

app/upgrades/v22/upgrades.go

+		// For sake of simplicity, we restart the taker fee tracker accounting height.
+		keepers.PoolManagerKeeper.SetTakerFeeTrackerStartHeight(ctx, ctx.BlockHeight())


Shouldn't we be keeping continuinity? THis is going to be much harder for every integrator for the future if we don't, no?

Implemented here f19b819

@ValarDragon would appreciate an ack on the implementation so it doesn't get swept under the rug when merging

ValarDragon · 2024-01-07T18:39:50Z

osmoutils/coin_helper.go

@@ -127,3 +127,16 @@ func ConvertCoinArrayToCoins(coinArray []sdk.Coin) sdk.Coins {
 	}
 	return coins
 }
+
+func ConvertCoinsToCoinArray(coins sdk.Coins) []sdk.Coin {


Im pretty sure you can do a type cast for this, seeing as sdk.Coins is just a type alias.

You are correct, calling []sdk.Coin(coinsObject) works, will implement this instead

Done here ba0c68e

ValarDragon · 2024-01-07T18:40:46Z

osmoutils/store_helper.go

+func GetCoinByDenomFromPrefix(ctx sdk.Context, storeKey storetypes.StoreKey, storePrefix []byte, denom string) (sdk.Coin, error) {
+	store := prefix.NewStore(ctx.KVStore(storeKey), storePrefix)
+	key := append(storePrefix, []byte(denom)...)
+
+	bz := store.Get(key)
+	if len(bz) == 0 {
+		return sdk.NewCoin(denom, osmomath.ZeroInt()), nil
+	}
+
+	coin := sdk.Coin{}
+	if err := coin.Unmarshal(bz); err != nil {
+		return sdk.NewCoin(denom, osmomath.ZeroInt()), err
+	}
+
+	return coin, nil
+}


Shouldn't we only marshal the amount, since the key contains the denom?

@ValarDragon Is there performance improvement if we just unmarshal the amount and then from that create a coin to return?

yeah:

less data written/read

(and therefore gas)

Two things:

Modified the key to use a separator for easier parsing of the denom from the key

Did not modify protorev's kvstore since we are not changing their store like we are the taker fee store. Otherwise I would need to recreate the store to store ints instead of coins, which feels out of scope of this PR

a27e3c8
a582fa1
c3f306d
c665113

…ified dependencies locally

ValarDragon · 2024-01-09T20:59:25Z

osmoutils/store_helper.go

+	store := prefix.NewStore(ctx.KVStore(storeKey), storePrefix)
+	key := []byte(fmt.Sprintf("%s%s%s", storePrefix, "|", []byte(denom)))


were double prefixing by the store prefix, resulting in the odd behavior

czarcas7ic added 3 commits January 3, 2024 13:03

remove tx fee tracker

2e919c6

add change log

3785022

track array of coins for performance

25b911d

github-actions bot added C:x/txfees C:app-wiring Changes to the app folder C:x/poolmanager labels Jan 4, 2024

Merge branch 'main' into adam/protorev-tracker-perf

3f217b9

czarcas7ic added the V:state/breaking State machine breaking PR label Jan 4, 2024

czarcas7ic and others added 5 commits January 3, 2024 21:54

changelog

ccf65fb

lints

2d88b49

lints

91d647e

fixes

867a1d0

Merge branch 'main' into adam/protorev-tracker-perf

b98972c

czarcas7ic marked this pull request as ready for review January 5, 2024 20:38

czarcas7ic requested review from p0mvn, mattverse and AlpinYukseloglu as code owners January 5, 2024 20:38

czarcas7ic commented Jan 5, 2024

View reviewed changes

Auto: update go.mod after push to adam/protorev-tracker-perf that mod…

bc9d9b0

…ified dependencies locally

czarcas7ic closed this Jan 5, 2024

czarcas7ic reopened this Jan 5, 2024

czarcas7ic marked this pull request as draft January 5, 2024 20:51

fix tests

12c0f74

github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Jan 5, 2024

czarcas7ic commented Jan 5, 2024

View reviewed changes

ValarDragon reviewed Jan 7, 2024

View reviewed changes

czarcas7ic and others added 5 commits January 7, 2024 12:05

remove osmoutils method for coins to coin array

ba0c68e

Merge branch 'main' into adam/protorev-tracker-perf

166f039

continue tracking from same accounting height pre upgrade

f19b819

Merge branch 'main' into adam/protorev-tracker-perf

0a054b0

Auto: update go.mod after push to adam/protorev-tracker-perf that mod…

a59dd33

…ified dependencies locally

czarcas7ic closed this Jan 8, 2024

czarcas7ic reopened this Jan 8, 2024

ValarDragon approved these changes Jan 9, 2024

View reviewed changes

czarcas7ic and others added 4 commits January 9, 2024 10:15

update store helper

a27e3c8

Auto: update go.mod after push to adam/protorev-tracker-perf that mod…

673a360

…ified dependencies locally

Merge branch 'main' into adam/protorev-tracker-perf

ba52471

go mod updates

fd1a39d

czarcas7ic requested a review from nicolaslara as a code owner January 9, 2024 17:30

github-actions bot added the C:x/epochs label Jan 9, 2024

czarcas7ic added 5 commits January 9, 2024 11:55

just use int

a582fa1

go mod updates

46bd310

revert statistics.go

c3f306d

fix store key

c665113

update go mods

709b70d

ValarDragon reviewed Jan 9, 2024

View reviewed changes

czarcas7ic added 4 commits January 9, 2024 14:12

dont double append prefix

b83740e

update go mod

ac2f006

Merge branch 'main' into adam/protorev-tracker-perf

17cb7c0

go mod changes

1216b17

czarcas7ic merged commit 227c44c into main Jan 9, 2024
1 check passed

czarcas7ic deleted the adam/protorev-tracker-perf branch January 9, 2024 21:36

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

Broken Links Detected #7319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(perf): protorev tracker coin array #7240

chore(perf): protorev tracker coin array #7240

czarcas7ic commented Jan 4, 2024 •

edited

Loading

czarcas7ic Jan 5, 2024

czarcas7ic Jan 5, 2024

czarcas7ic Jan 5, 2024

czarcas7ic Jan 5, 2024

czarcas7ic Jan 5, 2024

czarcas7ic Jan 5, 2024

czarcas7ic Jan 5, 2024

ValarDragon Jan 7, 2024

czarcas7ic Jan 8, 2024

ValarDragon Jan 7, 2024

czarcas7ic Jan 7, 2024

czarcas7ic Jan 7, 2024

ValarDragon Jan 7, 2024

czarcas7ic Jan 7, 2024

ValarDragon Jan 8, 2024

czarcas7ic Jan 9, 2024 •

edited

Loading

ValarDragon Jan 9, 2024

czarcas7ic Jan 9, 2024

ValarDragon Jan 9, 2024

		// For sake of simplicity, we restart the taker fee tracker accounting height.
		keepers.PoolManagerKeeper.SetTakerFeeTrackerStartHeight(ctx, ctx.BlockHeight())

		store := prefix.NewStore(ctx.KVStore(storeKey), storePrefix)
		key := []byte(fmt.Sprintf("%s%s%s", storePrefix, "\|", []byte(denom)))

chore(perf): protorev tracker coin array #7240

chore(perf): protorev tracker coin array #7240

Conversation

czarcas7ic commented Jan 4, 2024 • edited Loading

What is the purpose of the change

Testing and Verifying

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

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

czarcas7ic Jan 9, 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

czarcas7ic commented Jan 4, 2024 •

edited

Loading

czarcas7ic Jan 9, 2024 •

edited

Loading