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

fix(ProtoRev): Parameterizing Pool Type Information #5948

Merged
merged 27 commits into from
Aug 7, 2023
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#5831](https://github.com/osmosis-labs/osmosis/pull/5831) Fix superfluid_delegations query
* [#5835](https://github.com/osmosis-labs/osmosis/pull/5835) Fix println's for "amountZeroInRemainingBigDec before fee" making it into production
* [#5841] (https://github.com/osmosis-labs/osmosis/pull/5841) Fix protorev's out of gas erroring of the user's transcation.
* [#5930] (https://github.com/osmosis-labs/osmosis/pull/5930) Updating Protorev Binary Search Range Logic with CL Pools

### Misc Improvements

Expand All @@ -63,6 +64,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#5855](https://github.com/osmosis-labs/osmosis/pull/5855) feat(x/cosmwasmpool): Sending token_in_max_amount to the contract before running contract msg
* [#5893] (https://github.com/osmosis-labs/osmosis/pull/5893) Export createPosition method in CL so other modules can use it in testing
* [#5870] (https://github.com/osmosis-labs/osmosis/pull/5870) Remove v14/ separator in protorev rest endpoints
* [#5948] (https://github.com/osmosis-labs/osmosis/pull/5948) Parameterizing Pool Type Information in Protorev

### Minor improvements & Bug Fixes

Expand Down
8 changes: 7 additions & 1 deletion app/keepers/keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,13 @@ func (appKeepers *AppKeepers) InitNormalKeepers(
protorevKeeper := protorevkeeper.NewKeeper(
appCodec, appKeepers.keys[protorevtypes.StoreKey],
appKeepers.GetSubspace(protorevtypes.ModuleName),
appKeepers.AccountKeeper, appKeepers.BankKeeper, appKeepers.GAMMKeeper, appKeepers.EpochsKeeper, appKeepers.PoolManagerKeeper)
appKeepers.AccountKeeper,
appKeepers.BankKeeper,
appKeepers.GAMMKeeper,
appKeepers.EpochsKeeper,
appKeepers.PoolManagerKeeper,
appKeepers.ConcentratedLiquidityKeeper,
)
appKeepers.ProtoRevKeeper = &protorevKeeper

txFeesKeeper := txfeeskeeper.NewKeeper(
Expand Down
7 changes: 1 addition & 6 deletions app/upgrades/v17/upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,7 @@ func CreateUpgradeHandler(
}

// Reset the pool weights upon upgrade. This will add support for CW pools on ProtoRev.
keepers.ProtoRevKeeper.SetPoolWeights(ctx, types.PoolWeights{
BalancerWeight: 1,
StableWeight: 4,
ConcentratedWeight: 300,
CosmwasmWeight: 300,
})
keepers.ProtoRevKeeper.SetInfoByPoolType(ctx, types.DefaultPoolTypeInfo)

return migrations, nil
}
Expand Down
4 changes: 0 additions & 4 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -950,10 +950,6 @@ github.com/osmosis-labs/go-mutesting v0.0.0-20221208041716-b43bcd97b3b3 h1:Ylmch
github.com/osmosis-labs/go-mutesting v0.0.0-20221208041716-b43bcd97b3b3/go.mod h1:lV6KnqXYD/ayTe7310MHtM3I2q8Z6bBfMAi+bhwPYtI=
github.com/osmosis-labs/osmosis/osmomath v0.0.3-dev.0.20230629191111-f375469de8b6 h1:Kmkx5Rh72+LB8AL6dc6fZA+IVR0INu0YIiMF2ScDhaQ=
github.com/osmosis-labs/osmosis/osmomath v0.0.3-dev.0.20230629191111-f375469de8b6/go.mod h1:JTym95/bqrSnG5MPcXr1YDhv43JdCeo3p+iDbazoX68=
github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230709024311-81c831b050de h1:W2lMduMgpNA5zheEIIialw08n1pWJ44Y4t2F924tpDU=
github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230709024311-81c831b050de/go.mod h1:Pl8Nzx6O6ow/+aqfMoMSz4hX+zz6RrnDYsooptECGxM=
github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230715164027-b45d8bd42434 h1:MXPrA3sDtqOHYUa9zl4HMGMW+IJwGMqUf6+Hl9nhrCA=
github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230715164027-b45d8bd42434/go.mod h1:Pl8Nzx6O6ow/+aqfMoMSz4hX+zz6RrnDYsooptECGxM=
github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230728163612-426afac90c44 h1:UOaBVxEMMv2FS1znU7kHBdtSeZQIjnmXL4r9r19XyBo=
github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230728163612-426afac90c44/go.mod h1:Pl8Nzx6O6ow/+aqfMoMSz4hX+zz6RrnDYsooptECGxM=
github.com/osmosis-labs/osmosis/x/epochs v0.0.0-20230328024000-175ec88e4304 h1:RIrWLzIiZN5Xd2JOfSOtGZaf6V3qEQYg6EaDTAkMnCo=
Expand Down
8 changes: 4 additions & 4 deletions proto/osmosis/protorev/v1beta1/genesis.proto
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ 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.
PoolWeights pool_weights = 4 [
// Information that is used to estimate execution time / gas
// consumption of a swap on a given pool type.
InfoByPoolType info_by_pool_type = 4 [
(gogoproto.nullable) = false,
(gogoproto.moretags) = "yaml:\"pool_weights\""
(gogoproto.moretags) = "yaml:\"info_by_pool_type\""
Copy link
Member

Choose a reason for hiding this comment

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

If we are renaming the proto field, we should depreciate this field and add a new one

Copy link
Member

Choose a reason for hiding this comment

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

(Or reserve!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, add a note that this is deprecated and adding a new field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added back the previous field with a note that it is being deprecated.

];
// The number of days since module genesis.
uint64 days_since_module_genesis = 5
Expand Down
67 changes: 53 additions & 14 deletions proto/osmosis/protorev/v1beta1/protorev.proto
Original file line number Diff line number Diff line change
Expand Up @@ -73,23 +73,62 @@ message RouteStatistics {
repeated uint64 route = 3 [ (gogoproto.moretags) = "yaml:\"route\"" ];
}

// PoolWeights contains the weights of all of the different pool types. This
// distinction is made and necessary because the execution time ranges
// significantly between the different pool types. Each weight roughly
// corresponds to the amount of time (in ms) it takes to execute a swap on that
// pool type.
message PoolWeights {
// InfoByPoolType contains information pertaining to how expensive (in terms of
// gas and time) it is to execute a swap on a given pool type. This distinction
// is made and necessary because the execution time ranges significantly between
// the different pool types.
message InfoByPoolType {
// The stable pool info
StablePoolInfo stable = 1 [
(gogoproto.moretags) = "yaml:\"stable\"",
(gogoproto.nullable) = false
];
// The balancer pool info
BalancerPoolInfo balancer = 2 [
(gogoproto.moretags) = "yaml:\"balancer\"",
(gogoproto.nullable) = false
];
// The concentrated pool info
ConcentratedPoolInfo concentrated = 3 [
(gogoproto.moretags) = "yaml:\"concentrated\"",
(gogoproto.nullable) = false
];
// The cosmwasm pool info
CosmwasmPoolInfo cosmwasm = 4 [
(gogoproto.moretags) = "yaml:\"cosmwasm\"",
(gogoproto.nullable) = false
];
}

// StablePoolInfo contains meta data pertaining to a stableswap pool type.
message StablePoolInfo {
// The weight of a stableswap pool
uint64 stable_weight = 1 [ (gogoproto.moretags) = "yaml:\"stable_weight\"" ];
uint64 weight = 1 [ (gogoproto.moretags) = "yaml:\"weight\"" ];
}

// BalancerPoolInfo contains meta data pertaining to a balancer pool type.
message BalancerPoolInfo {
// The weight of a balancer pool
uint64 balancer_weight = 2
[ (gogoproto.moretags) = "yaml:\"balancer_weight\"" ];
uint64 weight = 1 [ (gogoproto.moretags) = "yaml:\"weight\"" ];
}

// ConcentratedPoolInfo contains meta data pertaining to a concentrated pool
// type.
message ConcentratedPoolInfo {
// The weight of a concentrated pool
uint64 concentrated_weight = 3
[ (gogoproto.moretags) = "yaml:\"concentrated_weight\"" ];
// The weight of a cosmwasm pool
uint64 cosmwasm_weight = 4
[ (gogoproto.moretags) = "yaml:\"cosmwasm_weight\"" ];
uint64 weight = 1 [ (gogoproto.moretags) = "yaml:\"weight\"" ];
// The maximum number of ticks we can move when rebalancing
uint64 max_ticks_crossed = 2
[ (gogoproto.moretags) = "yaml:\"max_ticks_crossed\"" ];
}

// CosmwasmPoolInfo contains meta data pertaining to a cosmwasm pool type.
message CosmwasmPoolInfo {
// The weight of a cosmwasm pool (by contract address)
map<string, uint64> weight_map = 1 [
Copy link
Contributor Author

@davidterpay davidterpay Aug 2, 2023

Choose a reason for hiding this comment

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

I'm not sure if using a map is problematic for app hashing. We do no iterations on the map itself in the module. The primary use case is determining the number of pool weight of a given cosmwasm pool (which may vary across applications which is why its mapped to contract addresses).

Copy link
Member

Choose a reason for hiding this comment

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

@ValarDragon can you verify the safety of this?

Copy link
Member

Choose a reason for hiding this comment

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

We had instances where we tried to use maps quite a while ago, I think the decision back then was to try avoid using maps for extra safety, would it be possible to change it so tht we use repeated structs instead here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. ty for confirming

(gogoproto.moretags) = "yaml:\"weight_map\"",
(gogoproto.nullable) = false
];
}

// BaseDenom represents a single base denom that the module uses for its
Expand Down
31 changes: 16 additions & 15 deletions proto/osmosis/protorev/v1beta1/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,11 @@ service Query {
option (google.api.http).get = "/osmosis/protorev/developer_account";
}

// GetProtoRevPoolWeights queries the weights of each pool type currently
// being used by the module
rpc GetProtoRevPoolWeights(QueryGetProtoRevPoolWeightsRequest)
returns (QueryGetProtoRevPoolWeightsResponse) {
option (google.api.http).get = "/osmosis/protorev/pool_weights";
// GetProtoRevInfoByPoolType queries pool type information that is currently
// being utilized by the module
rpc GetProtoRevInfoByPoolType(QueryGetProtoRevInfoByPoolTypeRequest)
returns (QueryGetProtoRevInfoByPoolTypeResponse) {
option (google.api.http).get = "/osmosis/protorev/info_by_pool_type";
}

// GetProtoRevMaxPoolPointsPerTx queries the maximum number of pool points
Expand Down Expand Up @@ -242,16 +242,17 @@ message QueryGetProtoRevDeveloperAccountResponse {
[ (gogoproto.moretags) = "yaml:\"developer_account\"" ];
}

// QueryGetProtoRevPoolWeightsRequest is request type for the
// Query/GetProtoRevPoolWeights RPC method.
message QueryGetProtoRevPoolWeightsRequest {}

// QueryGetProtoRevPoolWeightsResponse is response type for the
// Query/GetProtoRevPoolWeights RPC method.
message QueryGetProtoRevPoolWeightsResponse {
// pool_weights is a list of all of the pool weights
PoolWeights pool_weights = 1 [
(gogoproto.moretags) = "yaml:\"pool_weights\"",
// QueryGetProtoRevInfoByPoolTypeRequest is request type for the
// Query/GetProtoRevInfoByPoolType RPC method.
message QueryGetProtoRevInfoByPoolTypeRequest {}

// QueryGetProtoRevInfoByPoolTypeResponse is response type for the
// Query/GetProtoRevInfoByPoolType RPC method.
message QueryGetProtoRevInfoByPoolTypeResponse {
// InfoByPoolType contains all information pertaining to how different
// pool types are handled by the module.
InfoByPoolType info_by_pool_type = 1 [
(gogoproto.moretags) = "yaml:\"info_by_pool_type\"",
(gogoproto.nullable) = false
];
}
Expand Down
25 changes: 13 additions & 12 deletions proto/osmosis/protorev/v1beta1/tx.proto
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,11 @@ service Msg {
"/osmosis/protorev/set_max_pool_points_per_block";
};

// SetPoolWeights sets the weights of each pool type in the store. Can only be
// called by the admin account.
rpc SetPoolWeights(MsgSetPoolWeights) returns (MsgSetPoolWeightsResponse) {
option (google.api.http).post = "/osmosis/protorev/set_pool_weights";
// SetInfoByPoolType sets the pool type information needed to make smart
// assumptions about swapping on different pool types
rpc SetInfoByPoolType(MsgSetInfoByPoolType)
returns (MsgSetInfoByPoolTypeResponse) {
option (google.api.http).post = "/osmosis/protorev/set_info_by_pool_type";
};

// SetBaseDenoms sets the base denoms that will be used to create cyclic
Expand Down Expand Up @@ -90,24 +91,24 @@ message MsgSetDeveloperAccount {
// type.
message MsgSetDeveloperAccountResponse {}

// MsgSetPoolWeights defines the Msg/SetPoolWeights request type.
message MsgSetPoolWeights {
option (amino.name) = "osmosis/MsgSetPoolWeights";
// MsgSetInfoByPoolType defines the Msg/SetInfoByPoolType request type.
message MsgSetInfoByPoolType {
option (amino.name) = "osmosis/MsgSetInfoByPoolType";

// admin is the account that is authorized to set the pool weights.
string admin = 1 [
(gogoproto.moretags) = "yaml:\"admin\"",
(cosmos_proto.scalar) = "cosmos.AddressString"
];
// pool_weights is the list of pool weights to set.
PoolWeights pool_weights = 2 [
(gogoproto.moretags) = "yaml:\"pool_weights\"",
// info_by_pool_type contains information about the pool types.
InfoByPoolType info_by_pool_type = 2 [
(gogoproto.moretags) = "yaml:\"info_by_pool_type\"",
(gogoproto.nullable) = false
];
}

// MsgSetPoolWeightsResponse defines the Msg/SetPoolWeights response type.
message MsgSetPoolWeightsResponse {}
// MsgSetInfoByPoolTypeResponse defines the Msg/SetInfoByPoolType response type.
message MsgSetInfoByPoolTypeResponse {}

// MsgSetMaxPoolPointsPerTx defines the Msg/SetMaxPoolPointsPerTx request type.
message MsgSetMaxPoolPointsPerTx {
Expand Down
12 changes: 6 additions & 6 deletions tests/e2e/configurer/chain/queries.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,20 +118,20 @@ func (n *NodeConfig) QueryProtoRevDeveloperAccount() (sdk.AccAddress, error) {
return account, nil
}

// QueryProtoRevPoolWeights gets the pool point weights of the module.
func (n *NodeConfig) QueryProtoRevPoolWeights() (protorevtypes.PoolWeights, error) {
path := "/osmosis/protorev/pool_weights"
// QueryProtoRevInfoByPoolType gets information on how the module handles different pool types.
func (n *NodeConfig) QueryProtoRevInfoByPoolType() (*protorevtypes.InfoByPoolType, error) {
path := "/osmosis/protorev/info_by_pool_type"

bz, err := n.QueryGRPCGateway(path)
if err != nil {
return protorevtypes.PoolWeights{}, err
return nil, err
}

// nolint: staticcheck
var response protorevtypes.QueryGetProtoRevPoolWeightsResponse
var response protorevtypes.QueryGetProtoRevInfoByPoolTypeResponse
err = util.Cdc.UnmarshalJSON(bz, &response)
require.NoError(n.t, err) // this error should not happen
return response.PoolWeights, nil
return &response.InfoByPoolType, nil
}

// QueryProtoRevMaxPoolPointsPerTx gets the max pool points per tx of the module.
Expand Down
6 changes: 3 additions & 3 deletions tests/e2e/e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,10 @@ func (s *IntegrationTestSuite) ProtoRev() {
s.Require().Error(err)

// The module should have pool weights by default.
poolWeights, err := chainANode.QueryProtoRevPoolWeights()
s.T().Logf("checking that the protorev module has pool weights on init: %v", poolWeights)
info, err := chainANode.QueryProtoRevInfoByPoolType()
s.T().Logf("checking that the protorev module has pool info on init: %v", info)
s.Require().NoError(err)
s.Require().NotNil(poolWeights)
s.Require().NotNil(info)

// The module should have max pool points per tx by default.
maxPoolPointsPerTx, err := chainANode.QueryProtoRevMaxPoolPointsPerTx()
Expand Down
12 changes: 6 additions & 6 deletions x/protorev/client/cli/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func NewCmdQuery() *cobra.Command {
osmocli.AddQueryCmd(cmd, types.NewQueryClient, NewQueryMaxPoolPointsPerBlockCmd)
osmocli.AddQueryCmd(cmd, types.NewQueryClient, NewQueryBaseDenomsCmd)
osmocli.AddQueryCmd(cmd, types.NewQueryClient, NewQueryEnabledCmd)
osmocli.AddQueryCmd(cmd, types.NewQueryClient, NewQueryPoolWeightsCmd)
osmocli.AddQueryCmd(cmd, types.NewQueryClient, NewQueryInfoByPoolTypeCmd)
osmocli.AddQueryCmd(cmd, types.NewQueryClient, NewQueryPoolCmd)

return cmd
Expand Down Expand Up @@ -141,12 +141,12 @@ func NewQueryEnabledCmd() (*osmocli.QueryDescriptor, *types.QueryGetProtoRevEnab
}, &types.QueryGetProtoRevEnabledRequest{}
}

// NewQueryPoolWeightsCmd returns the command to query the pool weights of protorev
func NewQueryPoolWeightsCmd() (*osmocli.QueryDescriptor, *types.QueryGetProtoRevPoolWeightsRequest) {
// NewQueryInfoByPoolTypeCmd returns the command to query the pool type info of protorev
func NewQueryInfoByPoolTypeCmd() (*osmocli.QueryDescriptor, *types.QueryGetProtoRevInfoByPoolTypeRequest) {
return &osmocli.QueryDescriptor{
Use: "pool-weights",
Short: "Query the pool weights used to determine how computationally expensive a route is",
}, &types.QueryGetProtoRevPoolWeightsRequest{}
Use: "info-by-pool-type",
Short: "Query the pool info used to determine how computationally expensive a route is",
Comment on lines +147 to +148
Copy link
Member

Choose a reason for hiding this comment

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

I assume there is an input right? I think this should be "info-by-pool-type [pool-type]"

Copy link
Contributor Author

@davidterpay davidterpay Aug 7, 2023

Choose a reason for hiding this comment

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

yea the input is the json file with all of the correct pool info. This isnt used to update by a single pool type (although once the dependencies on a given pool type grow it might be worth migrating to separate message handlers).

I'll update the PR with the json example I used in testing.

}, &types.QueryGetProtoRevInfoByPoolTypeRequest{}
}

// NewQueryPoolCmd returns the command to query the pool id for a given denom pair stored via the highest liquidity method in ProtoRev
Expand Down
34 changes: 23 additions & 11 deletions x/protorev/client/cli/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func NewCmdTx() *cobra.Command {
osmocli.AddTxCmd(txCmd, CmdSetMaxPoolPointsPerBlock)
txCmd.AddCommand(
CmdSetDeveloperHotRoutes().BuildCommandCustomFn(),
CmdSetPoolWeights().BuildCommandCustomFn(),
CmdSetInfoByPoolType().BuildCommandCustomFn(),
CmdSetBaseDenoms().BuildCommandCustomFn(),
CmdSetProtoRevAdminAccountProposal(),
CmdSetProtoRevEnabledProposal(),
Expand Down Expand Up @@ -141,22 +141,34 @@ func CmdSetMaxPoolPointsPerBlock() (*osmocli.TxCliDesc, *types.MsgSetMaxPoolPoin
}, &types.MsgSetMaxPoolPointsPerBlock{}
}

// CmdSetPoolWeights implements the command to set the pool weights used to estimate execution costs
func CmdSetPoolWeights() *osmocli.TxCliDesc {
// CmdSetInfoByPoolType implements the command to set the pool information used throughout the module
func CmdSetInfoByPoolType() *osmocli.TxCliDesc {
desc := osmocli.TxCliDesc{
Use: "set-pool-weights [path/to/routes.json]",
Short: "set the protorev pool weights",
Long: `Must provide a json file with all the pool weights that will be set.
Use: "set-info-by-pool-type [path/to/pool_info.json]",
Short: "set the protorev pool type info",
Long: `Must provide a json file with all the pool info that will be set.
Sample json file:
{
"stable_weight" : 1,
"balancer_weight" : 1,
"concentrated_weight" : 1
"stable" : {
"weight" : 1,
},
"concentrated" : {
"weight" : 1,
"max_ticks_crossed": 10,
},
"balancer" : {
"weight" : 1,
},
"cosmwasm" : {
"weight_map" : {
"osmo123..." : 1,
},
},
}
`,
Example: fmt.Sprintf(`$ %s tx protorev set-pool-weights weights.json --from mykey`, version.AppName),
Example: fmt.Sprintf(`$ %s tx protorev set-info-by-pool-type pool_info.json --from mykey`, version.AppName),
NumArgs: 1,
ParseAndBuildMsg: BuildSetPoolWeightsMsg,
ParseAndBuildMsg: BuildSetInfoByPoolTypeMsg,
}

return &desc
Expand Down
Loading