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 1 commit
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
21 changes: 21 additions & 0 deletions proto/osmosis/protorev/v1beta1/params.proto
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,25 @@ message Params {
bool enabled = 1 [ (gogoproto.moretags) = "yaml:\"enabled\"" ];
// The admin account (settings manager) of the protorev module.
string admin = 2 [ (gogoproto.moretags) = "yaml:\"admin\"" ];
// All enabled base denominations for the protorev module.
repeated BaseDenom base_denoms = 3 [
(gogoproto.nullable) = false,
(gogoproto.moretags) = "yaml:\"base_denoms\""
];
}

// BaseDenom represents a single base denom that the module uses for its
// arbitrage trades. It contains the denom name alongside the step size of the
// binary search that is used to find the optimal swap amount
message BaseDenom {
// The denom i.e. name of the base denom (ex. uosmo)
string denom = 1 [ (gogoproto.moretags) = "yaml:\"denom\"" ];
// The step size of the binary search that is used to find the optimal swap
// amount
string step_size = 2 [

(gogoproto.customtype) = "cosmossdk.io/math.Int",
(gogoproto.nullable) = false,
(gogoproto.moretags) = "yaml:\"step_size\""
];
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved
}
16 changes: 0 additions & 16 deletions proto/osmosis/protorev/v1beta1/protorev.proto
Original file line number Diff line number Diff line change
Expand Up @@ -168,22 +168,6 @@ message WeightMap {
[ (gogoproto.moretags) = "yaml:\"contract_address\"" ];
}

// BaseDenom represents a single base denom that the module uses for its
// arbitrage trades. It contains the denom name alongside the step size of the
// binary search that is used to find the optimal swap amount
message BaseDenom {
// The denom i.e. name of the base denom (ex. uosmo)
string denom = 1 [ (gogoproto.moretags) = "yaml:\"denom\"" ];
// The step size of the binary search that is used to find the optimal swap
// amount
string step_size = 2 [

(gogoproto.customtype) = "cosmossdk.io/math.Int",
(gogoproto.nullable) = false,
(gogoproto.moretags) = "yaml:\"step_size\""
];
}

message AllProtocolRevenue {
osmosis.poolmanager.v1beta1.TakerFeesTracker taker_fees_tracker = 1 [
(gogoproto.moretags) = "yaml:\"taker_fees_tracker\"",
Expand Down
1 change: 1 addition & 0 deletions proto/osmosis/protorev/v1beta1/tx.proto
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import "amino/amino.proto";
import "google/api/annotations.proto";
import "osmosis/protorev/v1beta1/protorev.proto";
import "cosmos_proto/cosmos.proto";
import "osmosis/protorev/v1beta1/params.proto";

option go_package = "github.com/osmosis-labs/osmosis/v23/x/protorev/types";

Expand Down
5 changes: 1 addition & 4 deletions x/protorev/keeper/epoch_hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,7 @@ func (k Keeper) UpdatePools(ctx sdk.Context) error {
// baseDenomPools maps each base denom to a map of the highest liquidity pools paired with that base denom
// ex. {osmo -> {atom : 100, weth : 200}}
baseDenomPools := make(map[string]map[string]LiquidityPoolStruct)
baseDenoms, err := k.GetAllBaseDenoms(ctx)
if err != nil {
return err
}
baseDenoms := k.GetAllBaseDenoms(ctx)

// Delete any pools that currently exist in the store + initialize baseDenomPools
for _, baseDenom := range baseDenoms {
Expand Down
3 changes: 1 addition & 2 deletions x/protorev/keeper/epoch_hook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ func (s *KeeperTestSuite) TestEpochHook() {

totalNumberExpected := 0
expectedToSee := make(map[string]Pool)
baseDenoms, err := s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().NoError(err)
baseDenoms := s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
for _, pool := range s.pools {
// Module currently limited to two asset pools
// Instantiate asset and amounts for the pool
Expand Down
9 changes: 2 additions & 7 deletions x/protorev/keeper/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState types.GenesisState) {
// Configure the initial base denoms used for cyclic route building. The order of the list of base
// denoms is the order in which routes will be prioritized i.e. routes will be built and simulated in a
// first come first serve basis that is based on the order of the base denoms.
if err := k.SetBaseDenoms(ctx, genState.BaseDenoms); err != nil {
panic(err)
}
k.SetBaseDenoms(ctx, genState.BaseDenoms)

// Update the pools on genesis.
if err := k.UpdatePools(ctx); err != nil {
Expand Down Expand Up @@ -122,10 +120,7 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
genesis.TokenPairArbRoutes = routes

// Export the base denoms used for cyclic route building.
baseDenoms, err := k.GetAllBaseDenoms(ctx)
if err != nil {
panic(err)
}
baseDenoms := k.GetAllBaseDenoms(ctx)
genesis.BaseDenoms = baseDenoms

// Export the developer fees that have been collected.
Expand Down
3 changes: 1 addition & 2 deletions x/protorev/keeper/genesis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ func (s *KeeperTestSuite) TestInitGenesis() {
s.Require().Contains(tokenPairArbRoutes, route)
}

baseDenoms, err := s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().NoError(err)
baseDenoms := s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().Equal(len(baseDenoms), len(exportedGenesis.BaseDenoms))
for _, baseDenom := range exportedGenesis.BaseDenoms {
s.Require().Contains(baseDenoms, baseDenom)
Expand Down
5 changes: 1 addition & 4 deletions x/protorev/keeper/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,7 @@ func (q Querier) GetProtoRevBaseDenoms(c context.Context, req *types.QueryGetPro
return nil, status.Error(codes.InvalidArgument, "invalid request")
}
ctx := sdk.UnwrapSDKContext(c)
baseDenoms, err := q.Keeper.GetAllBaseDenoms(ctx)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
}
baseDenoms := q.Keeper.GetAllBaseDenoms(ctx)

return &types.QueryGetProtoRevBaseDenomsResponse{BaseDenoms: baseDenoms}, nil
}
Expand Down
3 changes: 1 addition & 2 deletions x/protorev/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -330,8 +330,7 @@ func (s *KeeperTestSuite) TestGetProtoRevMaxPoolPointsPerBlock() {
// TestGetProtoRevBaseDenoms tests the query to retrieve the base denoms
func (s *KeeperTestSuite) TestGetProtoRevBaseDenoms() {
// base denoms already set in setup
baseDenoms, err := s.App.AppKeepers.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().NoError(err)
baseDenoms := s.App.AppKeepers.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)

req := &types.QueryGetProtoRevBaseDenomsRequest{}
res, err := s.queryClient.GetProtoRevBaseDenoms(sdk.WrapSDKContext(s.Ctx), req)
Expand Down
6 changes: 1 addition & 5 deletions x/protorev/keeper/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,7 @@ func (k Keeper) StoreJoinExitPoolSwaps(ctx sdk.Context, sender sdk.AccAddress, p
// AfterPoolCreatedWithCoins checks if the new pool should be stored as the highest liquidity pool
// for any of the base denoms, and stores it if so.
func (k Keeper) AfterPoolCreatedWithCoins(ctx sdk.Context, poolId uint64) {
baseDenoms, err := k.GetAllBaseDenoms(ctx)
if err != nil {
ctx.Logger().Error("Protorev error getting base denoms in AfterCFMMPoolCreated hook: " + err.Error())
return
}
baseDenoms := k.GetAllBaseDenoms(ctx)

baseDenomMap := make(map[string]bool)
for _, baseDenom := range baseDenoms {
Expand Down
5 changes: 2 additions & 3 deletions x/protorev/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,7 @@ func (s *KeeperTestSuite) SetupTest() {
StepSize: osmomath.NewInt(1_000_000),
},
}
err := s.App.ProtoRevKeeper.SetBaseDenoms(s.Ctx, baseDenomPriorities)
s.Require().NoError(err)
s.App.ProtoRevKeeper.SetBaseDenoms(s.Ctx, baseDenomPriorities)

encodingConfig := osmosisapp.MakeEncodingConfig()
s.clientCtx = client.Context{}.
Expand Down Expand Up @@ -151,7 +150,7 @@ func (s *KeeperTestSuite) SetupTest() {

// Set the Admin Account
s.adminAccount = apptesting.CreateRandomAccounts(1)[0]
err = protorev.HandleSetProtoRevAdminAccount(s.Ctx, *s.App.ProtoRevKeeper, &types.SetProtoRevAdminAccountProposal{Account: s.adminAccount.String()})
err := protorev.HandleSetProtoRevAdminAccount(s.Ctx, *s.App.ProtoRevKeeper, &types.SetProtoRevAdminAccountProposal{Account: s.adminAccount.String()})
s.Require().NoError(err)

queryHelper := baseapp.NewQueryServerTestHelper(s.Ctx, s.App.InterfaceRegistry())
Expand Down
15 changes: 5 additions & 10 deletions x/protorev/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,22 +139,17 @@ func (m MsgServer) SetBaseDenoms(c context.Context, msg *types.MsgSetBaseDenoms)
}

// Get the old base denoms
baseDenoms, err := m.k.GetAllBaseDenoms(ctx)
if err != nil {
return nil, err
}
oldBaseDenoms := m.k.GetAllBaseDenoms(ctx)

// Delete all pools associated with the base denoms
for _, baseDenom := range baseDenoms {
for _, baseDenom := range oldBaseDenoms {
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.

// // Delete the old base denoms
// m.k.DeleteBaseDenoms(ctx)

if err := m.k.SetBaseDenoms(ctx, msg.BaseDenoms); err != nil {
return nil, err
}
m.k.SetBaseDenoms(ctx, msg.BaseDenoms)

// Update all of the pools
if err := m.k.UpdatePools(ctx); err != nil {
Expand Down
3 changes: 1 addition & 2 deletions x/protorev/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -655,8 +655,7 @@ func (s *KeeperTestSuite) TestMsgSetBaseDenoms() {
s.Require().NoError(err)
s.Require().Equal(response, &types.MsgSetBaseDenomsResponse{})

baseDenoms, err := s.App.AppKeepers.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().NoError(err)
baseDenoms := s.App.AppKeepers.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().Equal(testCase.baseDenoms, baseDenoms)
} else {
s.Require().Error(err)
Expand Down
68 changes: 39 additions & 29 deletions x/protorev/keeper/protorev.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,43 +76,53 @@ 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) {
baseDenoms := make([]types.BaseDenom, 0)
// // 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) {
// baseDenoms := make([]types.BaseDenom, 0)

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

defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
baseDenom := types.BaseDenom{}
err := baseDenom.Unmarshal(iterator.Value())
if err != nil {
return []types.BaseDenom{}, err
}
// defer iterator.Close()
// for ; iterator.Valid(); iterator.Next() {
// baseDenom := types.BaseDenom{}
// err := baseDenom.Unmarshal(iterator.Value())
// if err != nil {
// return []types.BaseDenom{}, err
// }

baseDenoms = append(baseDenoms, baseDenom)
}
// baseDenoms = append(baseDenoms, baseDenom)
// }

return baseDenoms, nil
}
// return 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 {
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixBaseDenoms)
// // 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 {
// store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixBaseDenoms)

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

bz, err := baseDenom.Marshal()
if err != nil {
return err
}
store.Set(key, bz)
}
// bz, err := baseDenom.Marshal()
// if err != nil {
// return err
// }
// store.Set(key, bz)
// }

return nil
// return nil
// }

func (k Keeper) GetAllBaseDenoms(ctx sdk.Context) []types.BaseDenom {
return k.GetParams(ctx).BaseDenoms
}

func (k Keeper) SetBaseDenoms(ctx sdk.Context, newBaseDenoms []types.BaseDenom) {
params := k.GetParams(ctx)
params.BaseDenoms = newBaseDenoms
k.SetParams(ctx, params)
}

// DeleteBaseDenoms deletes all of the base denoms
Expand Down
12 changes: 4 additions & 8 deletions x/protorev/keeper/protorev_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,24 +60,20 @@ func (s *KeeperTestSuite) TestDeleteAllTokenPairArbRoutes() {
// TestGetAllBaseDenoms tests the GetAllBaseDenoms, SetBaseDenoms, and DeleteBaseDenoms functions.
func (s *KeeperTestSuite) TestGetAllBaseDenoms() {
// Should be initialized on genesis
baseDenoms, err := s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().NoError(err)
baseDenoms := s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().Equal(3, len(baseDenoms))
s.Require().Equal(baseDenoms[0].Denom, types.OsmosisDenomination)
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)
baseDenoms = s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
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)
baseDenoms, err = s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().NoError(err)
s.App.ProtoRevKeeper.SetBaseDenoms(s.Ctx, []types.BaseDenom{{Denom: "osmo"}, {Denom: "atom"}, {Denom: "weth"}})
baseDenoms = s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
s.Require().Equal(3, len(baseDenoms))
s.Require().Equal(baseDenoms[0].Denom, "osmo")
s.Require().Equal(baseDenoms[1].Denom, "atom")
Expand Down
5 changes: 1 addition & 4 deletions x/protorev/keeper/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,7 @@ func (k Keeper) BuildHotRoute(ctx sdk.Context, route types.Route, poolId uint64)
// and routes are built in a greedy manner.
func (k Keeper) BuildHighestLiquidityRoutes(ctx sdk.Context, tokenIn, tokenOut string, poolId uint64) ([]RouteMetaData, error) {
routes := make([]RouteMetaData, 0)
baseDenoms, err := k.GetAllBaseDenoms(ctx)
if err != nil {
return routes, err
}
baseDenoms := k.GetAllBaseDenoms(ctx)

// Iterate through all denoms greedily. When simulating and executing trades, routes that are closer to the beginning of the list
// have priority over those that are later in the list. This way we can build routes that are more likely to succeed and bring in
Expand Down
Loading