Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(perf): protorev tracker coin array #7240

Merged
merged 42 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
2e919c6
remove tx fee tracker
czarcas7ic Jan 3, 2024
3785022
add change log
czarcas7ic Jan 3, 2024
25b911d
track array of coins for performance
czarcas7ic Jan 4, 2024
3f217b9
Merge branch 'main' into adam/protorev-tracker-perf
czarcas7ic Jan 4, 2024
ccf65fb
changelog
czarcas7ic Jan 4, 2024
2d88b49
lints
czarcas7ic Jan 4, 2024
91d647e
lints
czarcas7ic Jan 4, 2024
867a1d0
fixes
czarcas7ic Jan 5, 2024
b98972c
Merge branch 'main' into adam/protorev-tracker-perf
czarcas7ic Jan 5, 2024
bc9d9b0
Auto: update go.mod after push to adam/protorev-tracker-perf that mod…
invalid-email-address Jan 5, 2024
12c0f74
fix tests
czarcas7ic Jan 5, 2024
ef5176b
event
czarcas7ic Jan 5, 2024
2e78405
nil error
czarcas7ic Jan 5, 2024
69c2df1
expect accounting height to always be exported
czarcas7ic Jan 5, 2024
e3c0e33
range backwards and add test
czarcas7ic Jan 6, 2024
ee425f0
reduce code duplication
czarcas7ic Jan 6, 2024
759831d
Auto: update go.mod after push to adam/protorev-tracker-perf that mod…
invalid-email-address Jan 6, 2024
7a09aef
init image should not check seq number
czarcas7ic Jan 6, 2024
11013db
revert change to proto rev bc might expect error case
czarcas7ic Jan 6, 2024
286e914
revert init tag for now
czarcas7ic Jan 6, 2024
68bf4ec
config tag
czarcas7ic Jan 6, 2024
a16bea6
lint spelling
czarcas7ic Jan 6, 2024
dfca598
init tag
czarcas7ic Jan 6, 2024
6d6cd16
revert init image
czarcas7ic Jan 6, 2024
ba0c68e
remove osmoutils method for coins to coin array
czarcas7ic Jan 7, 2024
166f039
Merge branch 'main' into adam/protorev-tracker-perf
czarcas7ic Jan 7, 2024
f19b819
continue tracking from same accounting height pre upgrade
czarcas7ic Jan 8, 2024
0a054b0
Merge branch 'main' into adam/protorev-tracker-perf
czarcas7ic Jan 8, 2024
a59dd33
Auto: update go.mod after push to adam/protorev-tracker-perf that mod…
invalid-email-address Jan 8, 2024
a27e3c8
update store helper
czarcas7ic Jan 9, 2024
673a360
Auto: update go.mod after push to adam/protorev-tracker-perf that mod…
invalid-email-address Jan 9, 2024
ba52471
Merge branch 'main' into adam/protorev-tracker-perf
czarcas7ic Jan 9, 2024
fd1a39d
go mod updates
czarcas7ic Jan 9, 2024
a582fa1
just use int
czarcas7ic Jan 9, 2024
46bd310
go mod updates
czarcas7ic Jan 9, 2024
c3f306d
revert statistics.go
czarcas7ic Jan 9, 2024
c665113
fix store key
czarcas7ic Jan 9, 2024
709b70d
update go mods
czarcas7ic Jan 9, 2024
b83740e
dont double append prefix
czarcas7ic Jan 9, 2024
ac2f006
update go mod
czarcas7ic Jan 9, 2024
17cb7c0
Merge branch 'main' into adam/protorev-tracker-perf
czarcas7ic Jan 9, 2024
1216b17
go mod changes
czarcas7ic Jan 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
reduce code duplication
  • Loading branch information
czarcas7ic committed Jan 6, 2024
commit ee425f046e3a7df31cdd297df7dbb6098cacf834
60 changes: 60 additions & 0 deletions osmoutils/store_helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"github.com/cosmos/cosmos-sdk/store/prefix"
sdk "github.com/cosmos/cosmos-sdk/types"

storetypes "github.com/cosmos/cosmos-sdk/store/types"

"github.com/osmosis-labs/osmosis/osmomath"

"github.com/cosmos/cosmos-sdk/store"
Expand Down Expand Up @@ -222,3 +224,61 @@ func DeleteAllKeysFromPrefix(store store.KVStore, prefixKey []byte) {
prefixStore.Delete(iter.Key())
}
}

// GetCoinArrayFromPrefix returns all coins from the store that has the given prefix.
func GetCoinArrayFromPrefix(ctx sdk.Context, storeKey storetypes.StoreKey, storePrefix []byte) []sdk.Coin {
coinArray := make([]sdk.Coin, 0)

store := ctx.KVStore(storeKey)
iterator := sdk.KVStorePrefixIterator(store, storePrefix)

defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
bz := iterator.Value()
coin := sdk.Coin{}
if err := coin.Unmarshal(bz); err == nil {
coinArray = append(coinArray, coin)
}
}

return coinArray
}

// GetCoinByDenomFromPrefix returns the coin from the store that has the given prefix and denom.
// If the denom is not found, a zero coin is returned.
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
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah:

  • less data written/read
  • (and therefore gas)

Copy link
Member Author

@czarcas7ic czarcas7ic Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

  1. Modified the key to use a separator for easier parsing of the denom from the key
  2. 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


// IncreaseCoinByDenomFromPrefix increases the coin from the store that has the given prefix and denom by the specified amount.
func IncreaseCoinByDenomFromPrefix(ctx sdk.Context, storeKey storetypes.StoreKey, storePrefix []byte, denom string, increasedAmt osmomath.Int) error {
store := prefix.NewStore(ctx.KVStore(storeKey), storePrefix)
key := append(storePrefix, []byte(denom)...)

coin, err := GetCoinByDenomFromPrefix(ctx, storeKey, storePrefix, denom)
if err != nil {
return err
}

coin.Amount = coin.Amount.Add(increasedAmt)
bz, err := coin.Marshal()
if err != nil {
return err
}

store.Set(key, bz)
return nil
}
94 changes: 8 additions & 86 deletions x/poolmanager/protorev.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,118 +7,40 @@ import (
"github.com/osmosis-labs/osmosis/osmomath"
"github.com/osmosis-labs/osmosis/osmoutils"
"github.com/osmosis-labs/osmosis/v21/x/poolmanager/types"

"github.com/cosmos/cosmos-sdk/store/prefix"
)

// 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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

takerFeesForStakers := make([]sdk.Coin, 0)

store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, types.KeyTakerFeeStakersProtoRevArray)

defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
bz := iterator.Value()
takerFeeForStakers := sdk.Coin{}
if err := takerFeeForStakers.Unmarshal(bz); err == nil {
takerFeesForStakers = append(takerFeesForStakers, takerFeeForStakers)
}
}

return takerFeesForStakers
return osmoutils.GetCoinArrayFromPrefix(ctx, k.storeKey, types.KeyTakerFeeStakersProtoRevArray)
}

// GetTakerFeeTrackerForStakersByDenom returns the taker fee for stakers tracker for the specified denom that has been
// collected since the accounting height.
// collected since the accounting height. If the denom is not found, a zero coin is returned.
func (k Keeper) GetTakerFeeTrackerForStakersByDenom(ctx sdk.Context, denom string) (sdk.Coin, error) {
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyTakerFeeStakersProtoRevArray)
key := types.GetKeyPrefixTakerFeeStakersProtoRevByDenom(denom)

bz := store.Get(key)
if len(bz) == 0 {
return sdk.NewCoin(denom, osmomath.ZeroInt()), nil
}

takerFeeForStakers := sdk.Coin{}
if err := takerFeeForStakers.Unmarshal(bz); err != nil {
return sdk.NewCoin(denom, osmomath.ZeroInt()), err
}

return takerFeeForStakers, nil
return osmoutils.GetCoinByDenomFromPrefix(ctx, k.storeKey, types.KeyTakerFeeStakersProtoRevArray, denom)
}

// UpdateTakerFeeTrackerForStakersByDenom increases the take fee for stakers tracker for the specified denom by the specified amount.
func (k Keeper) UpdateTakerFeeTrackerForStakersByDenom(ctx sdk.Context, denom string, increasedAmt osmomath.Int) error {
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyTakerFeeStakersProtoRevArray)
key := types.GetKeyPrefixTakerFeeStakersProtoRevByDenom(denom)

takerFeeForStakers, _ := k.GetTakerFeeTrackerForStakersByDenom(ctx, denom)
takerFeeForStakers.Amount = takerFeeForStakers.Amount.Add(increasedAmt)
bz, err := takerFeeForStakers.Marshal()
if err != nil {
return err
}

store.Set(key, bz)
return nil
return osmoutils.IncreaseCoinByDenomFromPrefix(ctx, k.storeKey, types.KeyTakerFeeStakersProtoRevArray, denom, increasedAmt)
}

// GetTakerFeeTrackerForCommunityPool returns the taker fee for community pool tracker for all denoms that has been
// collected since the accounting height.
func (k Keeper) GetTakerFeeTrackerForCommunityPool(ctx sdk.Context) []sdk.Coin {
takerFeesForCommunityPool := make([]sdk.Coin, 0)

store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, types.KeyTakerFeeCommunityPoolProtoRevArray)

defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
bz := iterator.Value()
takerFeeForCommunityPool := sdk.Coin{}
if err := takerFeeForCommunityPool.Unmarshal(bz); err == nil {
takerFeesForCommunityPool = append(takerFeesForCommunityPool, takerFeeForCommunityPool)
}
}

return takerFeesForCommunityPool
return osmoutils.GetCoinArrayFromPrefix(ctx, k.storeKey, types.KeyTakerFeeCommunityPoolProtoRevArray)
}

// GetTakerFeeTrackerForCommunityPoolByDenom returns the taker fee for community pool tracker for the specified denom that has been
// collected since the accounting height.
// collected since the accounting height. If the denom is not found, a zero coin is returned.
func (k Keeper) GetTakerFeeTrackerForCommunityPoolByDenom(ctx sdk.Context, denom string) (sdk.Coin, error) {
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyTakerFeeCommunityPoolProtoRevArray)
key := types.GetKeyPrefixTakerFeeCommunityPoolProtoRevByDenom(denom)

bz := store.Get(key)
if len(bz) == 0 {
return sdk.NewCoin(denom, osmomath.ZeroInt()), nil
}

takerFeeForCommunityPool := sdk.Coin{}
if err := takerFeeForCommunityPool.Unmarshal(bz); err != nil {
return sdk.NewCoin(denom, osmomath.ZeroInt()), err
}

return takerFeeForCommunityPool, nil
return osmoutils.GetCoinByDenomFromPrefix(ctx, k.storeKey, types.KeyTakerFeeCommunityPoolProtoRevArray, denom)
}

// UpdateTakerFeeTrackerForCommunityPoolByDenom increases the take fee for community pool tracker for the specified denom by the specified amount.
func (k Keeper) UpdateTakerFeeTrackerForCommunityPoolByDenom(ctx sdk.Context, denom string, increasedAmt osmomath.Int) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need so many similar methods? What if we only had one that takes store prefix as an input and made short wrappers that would call this shared function?

This would reduce code duplication.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, added the logic to osmoutils here ee425f0

Lmk if you think I should just keep in poolmanger instead. I thought the logic could be extendable to any other time we need to track coins in the future, which I imagine will occur again.

store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyTakerFeeCommunityPoolProtoRevArray)
key := types.GetKeyPrefixTakerFeeCommunityPoolProtoRevByDenom(denom)

takerFeeForCommunityPool, _ := k.GetTakerFeeTrackerForCommunityPoolByDenom(ctx, denom)
takerFeeForCommunityPool.Amount = takerFeeForCommunityPool.Amount.Add(increasedAmt)
bz, err := takerFeeForCommunityPool.Marshal()
if err != nil {
return err
}

store.Set(key, bz)
return nil
return osmoutils.IncreaseCoinByDenomFromPrefix(ctx, k.storeKey, types.KeyTakerFeeCommunityPoolProtoRevArray, denom, increasedAmt)
}

// GetTakerFeeTrackerStartHeight gets the height from which we started accounting for taker fees.
Expand Down
8 changes: 0 additions & 8 deletions x/poolmanager/types/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,3 @@ func ParseDenomTradePairKey(key []byte) (denom0, denom1 string, err error) {

return denom0, denom1, nil
}

func GetKeyPrefixTakerFeeStakersProtoRevByDenom(denom string) []byte {
return append(KeyTakerFeeStakersProtoRevArray, []byte(denom)...)
}

func GetKeyPrefixTakerFeeCommunityPoolProtoRevByDenom(denom string) []byte {
return append(KeyTakerFeeCommunityPoolProtoRevArray, []byte(denom)...)
}
57 changes: 10 additions & 47 deletions x/protorev/keeper/statistics.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,41 +49,20 @@ func (k Keeper) IncrementNumberOfTrades(ctx sdk.Context) error {
return nil
}

// GetProfitsByDenom returns the profits made by the ProtoRev module for the given denom
func (k Keeper) GetProfitsByDenom(ctx sdk.Context, denom string) (sdk.Coin, error) {
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixProfitByDenom)
key := types.GetKeyPrefixProfitByDenom(denom)

bz := store.Get(key)
if len(bz) == 0 {
return sdk.NewCoin(denom, osmomath.ZeroInt()), fmt.Errorf("no profits for denom %s", denom)
}

profits := sdk.Coin{}
if err := profits.Unmarshal(bz); err != nil {
return sdk.NewCoin(denom, osmomath.ZeroInt()), err
}

return profits, nil
}

// GetAllProfits returns all of the profits made by the ProtoRev module.
func (k Keeper) GetAllProfits(ctx sdk.Context) []sdk.Coin {
profits := make([]sdk.Coin, 0)

store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, types.KeyPrefixProfitByDenom)
return osmoutils.GetCoinArrayFromPrefix(ctx, k.storeKey, types.KeyPrefixProfitByDenom)
}

defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
bz := iterator.Value()
profit := sdk.Coin{}
if err := profit.Unmarshal(bz); err == nil {
profits = append(profits, profit)
}
}
// GetProfitsByDenom returns the profits made by the ProtoRev module for the given denom.
// If the denom is not found, a zero coin is returned.
func (k Keeper) GetProfitsByDenom(ctx sdk.Context, denom string) (sdk.Coin, error) {
return osmoutils.GetCoinByDenomFromPrefix(ctx, k.storeKey, types.KeyPrefixProfitByDenom, denom)
}

return profits
// UpdateProfitsByDenom updates the profits made by the ProtoRev module for the given denom
func (k Keeper) UpdateProfitsByDenom(ctx sdk.Context, denom string, tradeProfit osmomath.Int) error {
return osmoutils.IncreaseCoinByDenomFromPrefix(ctx, k.storeKey, types.KeyPrefixProfitByDenom, denom, tradeProfit)
}

func (k Keeper) SetCyclicArbProfitTrackerValue(ctx sdk.Context, cyclicArbProfits sdk.Coins) {
Expand Down Expand Up @@ -126,22 +105,6 @@ func (k Keeper) SetCyclicArbProfitTrackerStartHeight(ctx sdk.Context, startHeigh
osmoutils.MustSet(ctx.KVStore(k.storeKey), types.KeyCyclicArbTrackerStartHeight, &gogotypes.Int64Value{Value: startHeight})
}

// UpdateProfitsByDenom updates the profits made by the ProtoRev module for the given denom
func (k Keeper) UpdateProfitsByDenom(ctx sdk.Context, denom string, tradeProfit osmomath.Int) error {
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixProfitByDenom)
key := types.GetKeyPrefixProfitByDenom(denom)

profits, _ := k.GetProfitsByDenom(ctx, denom)
profits.Amount = profits.Amount.Add(tradeProfit)
bz, err := profits.Marshal()
if err != nil {
return err
}

store.Set(key, bz)
return nil
}

// GetAllRoutes returns all of the routes that the ProtoRev module has traded on
func (k Keeper) GetAllRoutes(ctx sdk.Context) ([][]uint64, error) {
routes := make([][]uint64, 0)
Expand Down
5 changes: 0 additions & 5 deletions x/protorev/types/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,6 @@ func GetKeyPrefixRouteForTokenPair(tokenA, tokenB string) []byte {
return append(KeyPrefixTokenPairRoutes, []byte(tokenA+"|"+tokenB)...)
}

// Returns the key needed to fetch the profit by coin
func GetKeyPrefixProfitByDenom(denom string) []byte {
return append(KeyPrefixProfitByDenom, []byte(denom)...)
}

// Returns the key needed to fetch the number of trades by route
func GetKeyPrefixTradesByRoute(route []uint64) []byte {
return append(KeyPrefixTradesByRoute, CreateRouteKey(route)...)
Expand Down