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

perf(protorev): base denom as param #7508

Merged
merged 22 commits into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#7250](https://github.com/osmosis-labs/osmosis/pull/7250) Further filter spam gauges from epoch distribution.
* [#7472](https://github.com/osmosis-labs/osmosis/pull/7472) Refactor TWAP keys to only require a single key format. Significantly lowers TWAP-caused writes
* [#7499](https://github.com/osmosis-labs/osmosis/pull/7499) Slight speed/gas improvements to CL CreatePosition and AddToPosition
* [#7508](https://github.com/osmosis-labs/osmosis/pull/7508) Improve protorev performance by removing iterator and storing base denoms as a single object rather than an array.

## v23.0.0

Expand Down
12 changes: 12 additions & 0 deletions app/upgrades/v24/upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,18 @@ func CreateUpgradeHandler(
return nil, err
}

// We no longer use the base denoms array and instead use the repeated base denoms field for performance reasons.
// We retrieve the old base denoms array from the KVStore, delete the array from the KVStore, and set them as a repeated field in the new KVStore.
baseDenoms, err := keepers.ProtoRevKeeper.DeprecatedGetAllBaseDenoms(ctx)
if err != nil {
return nil, err
}
keepers.ProtoRevKeeper.DeprecatedDeleteBaseDenoms(ctx)
err = keepers.ProtoRevKeeper.SetBaseDenoms(ctx, baseDenoms)
if err != nil {
return nil, err
}

// Now that the TWAP keys are refactored, we can delete all time indexed TWAPs
// since we only need the pool indexed TWAPs.
keepers.TwapKeeper.DeleteAllHistoricalTimeIndexedTWAPs(ctx)
Expand Down
41 changes: 41 additions & 0 deletions app/upgrades/v24/upgrades_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/osmosis-labs/osmosis/osmoutils"
"github.com/osmosis-labs/osmosis/v23/app/apptesting"

protorevtypes "github.com/osmosis-labs/osmosis/v23/x/protorev/types"
"github.com/osmosis-labs/osmosis/v23/x/twap/types"
twaptypes "github.com/osmosis-labs/osmosis/v23/x/twap/types"
)
Expand All @@ -37,6 +38,9 @@ func TestUpgradeTestSuite(t *testing.T) {
func (s *UpgradeTestSuite) TestUpgrade() {
s.Setup()

// TWAP Setup
//

// Manually set up TWAP records indexed by both pool ID and time.
twapStoreKey := s.App.GetKey(twaptypes.ModuleName)
store := s.Ctx.KVStore(twapStoreKey)
Expand Down Expand Up @@ -75,11 +79,35 @@ func (s *UpgradeTestSuite) TestUpgrade() {
s.Require().Len(twapRecords, 1)
s.Require().Equal(twap, twapRecords[0])

// PROTOREV Setup
//

// Set the old KVStore base denoms
s.App.ProtoRevKeeper.DeprecatedSetBaseDenoms(s.Ctx, []protorevtypes.BaseDenom{
{Denom: protorevtypes.OsmosisDenomination, StepSize: osmomath.NewInt(1_000_000)},
{Denom: "atom", StepSize: osmomath.NewInt(1_000_000)},
{Denom: "weth", StepSize: osmomath.NewInt(1_000_000)}})
oldBaseDenoms, err := s.App.ProtoRevKeeper.DeprecatedGetAllBaseDenoms(s.Ctx)
s.Require().NoError(err)
s.Require().Equal(3, len(oldBaseDenoms))
s.Require().Equal(oldBaseDenoms[0].Denom, protorevtypes.OsmosisDenomination)
s.Require().Equal(oldBaseDenoms[1].Denom, "atom")
s.Require().Equal(oldBaseDenoms[2].Denom, "weth")

// The new KVStore should be set to the default
newBaseDenoms, err := s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().NoError(err)
s.Require().Equal(protorevtypes.DefaultBaseDenoms, newBaseDenoms)

// Run the upgrade
dummyUpgrade(s)
s.Require().NotPanics(func() {
s.App.BeginBlocker(s.Ctx, abci.RequestBeginBlock{})
})

// TWAP Tests
//

// TWAP records indexed by time should be completely removed.
twapRecords, err = osmoutils.GatherValuesFromStorePrefix(store, []byte(HistoricalTWAPTimeIndexPrefix), types.ParseTwapFromBz)
s.Require().NoError(err)
Expand All @@ -90,6 +118,19 @@ func (s *UpgradeTestSuite) TestUpgrade() {
s.Require().NoError(err)
s.Require().Len(twapRecords, 1)
s.Require().Equal(twap, twapRecords[0])

// PROTOREV Tests
//

// The new KVStore should return the old KVStore values
newBaseDenoms, err = s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().NoError(err)
s.Require().Equal(oldBaseDenoms, newBaseDenoms)

// The old KVStore base denoms should be deleted
oldBaseDenoms, err = s.App.ProtoRevKeeper.DeprecatedGetAllBaseDenoms(s.Ctx)
s.Require().NoError(err)
s.Require().Empty(oldBaseDenoms)
}

func dummyUpgrade(s *UpgradeTestSuite) {
Expand Down
11 changes: 5 additions & 6 deletions proto/osmosis/protorev/v1beta1/genesis.proto
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,13 @@ message GenesisState {
(gogoproto.nullable) = false,
(gogoproto.moretags) = "yaml:\"base_denoms\""
];
// The pool weights that are being used to calculate the weight (compute cost)
// of each route.
//
// DEPRECATED: This field is deprecated and will be removed in the next
// release. It is replaced by the `info_by_pool_type` field.
// DEPRECATED: The pool weights that are being used to calculate the weight
// (compute cost) of each route. This field is deprecated and will be removed
// in the next release. It is replaced by the `info_by_pool_type` field.
Copy link
Member Author

Choose a reason for hiding this comment

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

Drive by change, Skip team agreed this should be marked as deprecated.

Copy link
Member

@ValarDragon ValarDragon Feb 18, 2024

Choose a reason for hiding this comment

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

LGTM, though I'm confused why we don't just mark this as reserved? Whats the point of deprecating?

Copy link
Member Author

@czarcas7ic czarcas7ic Feb 20, 2024

Choose a reason for hiding this comment

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

No you are right we can reserve, I can make the change now

Copy link
Member Author

Choose a reason for hiding this comment

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

Reserved here a838fc1

PoolWeights pool_weights = 4 [
(gogoproto.nullable) = false,
(gogoproto.moretags) = "yaml:\"pool_weights\""
(gogoproto.moretags) = "yaml:\"pool_weights\"",
deprecated = true
];
// The number of days since module genesis.
uint64 days_since_module_genesis = 5
Expand Down
9 changes: 9 additions & 0 deletions proto/osmosis/protorev/v1beta1/protorev.proto
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,15 @@ message BaseDenom {
];
}

// BaseDenoms represents all of the base denoms that the module uses for its
// arbitrage trades.
message BaseDenoms {
repeated BaseDenom base_denoms = 1 [
(gogoproto.nullable) = false,
(gogoproto.moretags) = "yaml:\"base_denoms\""
];
}

message AllProtocolRevenue {
osmosis.poolmanager.v1beta1.TakerFeesTracker taker_fees_tracker = 1 [
(gogoproto.moretags) = "yaml:\"taker_fees_tracker\"",
Expand Down
3 changes: 0 additions & 3 deletions x/protorev/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,6 @@ func (m MsgServer) SetBaseDenoms(c context.Context, msg *types.MsgSetBaseDenoms)
m.k.DeleteAllPoolsForBaseDenom(ctx, baseDenom.Denom)
}

// Delete the old base denoms
m.k.DeleteBaseDenoms(ctx)
Comment on lines -152 to -153
Copy link
Member Author

Choose a reason for hiding this comment

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

We no longer need to delete the base denoms since we overwrite the single BaseDenoms value.


if err := m.k.SetBaseDenoms(ctx, msg.BaseDenoms); err != nil {
return nil, err
}
Expand Down
49 changes: 39 additions & 10 deletions x/protorev/keeper/protorev.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,13 @@ func (k Keeper) DeleteAllTokenPairArbRoutes(ctx sdk.Context) {
k.DeleteAllEntriesForKeyPrefix(ctx, types.KeyPrefixTokenPairRoutes)
}

// GetAllBaseDenoms returns all of the base denoms (sorted by priority in descending order) used to build cyclic arbitrage routes
func (k Keeper) GetAllBaseDenoms(ctx sdk.Context) ([]types.BaseDenom, error) {
// DeprecatedGetAllBaseDenoms returns all of the base denoms (sorted by priority in descending order) used to build cyclic arbitrage routes
// After v24 upgrade, this method should be deleted. We now use the param store.
func (k Keeper) DeprecatedGetAllBaseDenoms(ctx sdk.Context) ([]types.BaseDenom, error) {
baseDenoms := make([]types.BaseDenom, 0)

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

defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
Expand All @@ -97,13 +98,14 @@ func (k Keeper) GetAllBaseDenoms(ctx sdk.Context) ([]types.BaseDenom, error) {
return baseDenoms, nil
}

// SetBaseDenoms sets all of the base denoms used to build cyclic arbitrage routes. The base denoms priority
// DeprecatedSetBaseDenoms sets all of the base denoms used to build cyclic arbitrage routes. The base denoms priority
// order is going to match the order of the base denoms in the slice.
func (k Keeper) SetBaseDenoms(ctx sdk.Context, baseDenoms []types.BaseDenom) error {
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixBaseDenoms)
// After v24 upgrade, this method should be deleted. We now use the param store.
func (k Keeper) DeprecatedSetBaseDenoms(ctx sdk.Context, baseDenoms []types.BaseDenom) error {
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixDeprecatedBaseDenoms)

for i, baseDenom := range baseDenoms {
key := types.GetKeyPrefixBaseDenom(uint64(i))
key := types.DeprecatedGetKeyPrefixBaseDenom(uint64(i))

bz, err := baseDenom.Marshal()
if err != nil {
Expand All @@ -115,9 +117,36 @@ func (k Keeper) SetBaseDenoms(ctx sdk.Context, baseDenoms []types.BaseDenom) err
return nil
}

// DeleteBaseDenoms deletes all of the base denoms
func (k Keeper) DeleteBaseDenoms(ctx sdk.Context) {
k.DeleteAllEntriesForKeyPrefix(ctx, types.KeyPrefixBaseDenoms)
// DeprecatedDeleteBaseDenoms deletes all of the base denoms.
// After v24 upgrade, this method should be deleted. We now use the param store.
func (k Keeper) DeprecatedDeleteBaseDenoms(ctx sdk.Context) {
k.DeleteAllEntriesForKeyPrefix(ctx, types.KeyPrefixDeprecatedBaseDenoms)
}

// GetAllBaseDenoms returns all of the base denoms (sorted by priority in descending order) used to build cyclic arbitrage routes
func (k Keeper) GetAllBaseDenoms(ctx sdk.Context) ([]types.BaseDenom, error) {
store := ctx.KVStore(k.storeKey)
bz := store.Get(types.KeyPrefixBaseDenoms)
baseDenoms := types.BaseDenoms{}
err := baseDenoms.Unmarshal(bz)
if err != nil {
return []types.BaseDenom{}, err
}
return baseDenoms.BaseDenoms, nil
}

// SetBaseDenoms sets all of the base denoms used to build cyclic arbitrage routes. The base denoms priority
// order is going to match the order of the base denoms in the slice.
func (k Keeper) SetBaseDenoms(ctx sdk.Context, baseDenoms []types.BaseDenom) error {
newBaseDenoms := types.BaseDenoms{BaseDenoms: baseDenoms}
store := ctx.KVStore(k.storeKey)
test, err := newBaseDenoms.Marshal()
if err != nil {
return err
}

store.Set(types.KeyPrefixBaseDenoms, test)
return nil
}

// GetPoolForDenomPair returns the id of the highest liquidity pool between the base denom and the denom to match
Expand Down
6 changes: 0 additions & 6 deletions x/protorev/keeper/protorev_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,6 @@ func (s *KeeperTestSuite) TestGetAllBaseDenoms() {
s.Require().Equal(baseDenoms[1].Denom, "Atom")
s.Require().Equal(baseDenoms[2].Denom, "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7")

// Should be able to delete all base denoms
s.App.ProtoRevKeeper.DeleteBaseDenoms(s.Ctx)
baseDenoms, err = s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().NoError(err)
s.Require().Equal(0, len(baseDenoms))

// Should be able to set the base denoms
err = s.App.ProtoRevKeeper.SetBaseDenoms(s.Ctx, []types.BaseDenom{{Denom: "osmo"}, {Denom: "atom"}, {Denom: "weth"}})
s.Require().NoError(err)
Expand Down
Loading