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

[TRA-611] Get exponent from marketmap #2324

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 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
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,12 @@ export interface MarketParam {
* For example if `Exponent == -5` then a `Value` of `1,000,000,000`
* represents ``$10,000`. Therefore `10 ^ Exponent` represents the smallest
* price step (in dollars) that can be recorded.
*
* Deprecated since v7.1.x. This value is now determined from the marketmap.
*/

/** @deprecated */

exponent: number;
/**
* The minimum number of exchanges that should be reporting a live price for
Expand Down Expand Up @@ -58,8 +62,12 @@ export interface MarketParamSDKType {
* For example if `Exponent == -5` then a `Value` of `1,000,000,000`
* represents ``$10,000`. Therefore `10 ^ Exponent` represents the smallest
* price step (in dollars) that can be recorded.
*
* Deprecated since v7.1.x. This value is now determined from the marketmap.
*/

/** @deprecated */

exponent: number;
/**
* The minimum number of exchanges that should be reporting a live price for
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ export interface MarketPrice {
/**
* Static value. The exponent of the price. See the comment on the duplicate
* MarketParam field for more information.
*
* As of v7.1.x, this value is determined from the marketmap instead of
* needing to match the MarketParam field.
*/

exponent: number;
Expand All @@ -26,6 +29,9 @@ export interface MarketPriceSDKType {
/**
* Static value. The exponent of the price. See the comment on the duplicate
* MarketParam field for more information.
*
* As of v7.1.x, this value is determined from the marketmap instead of
* needing to match the MarketParam field.
*/

exponent: number;
Expand Down
4 changes: 3 additions & 1 deletion proto/dydxprotocol/prices/market_param.proto
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ message MarketParam {
// For example if `Exponent == -5` then a `Value` of `1,000,000,000`
// represents ``$10,000`. Therefore `10 ^ Exponent` represents the smallest
// price step (in dollars) that can be recorded.
sint32 exponent = 3;
//
// Deprecated since v7.1.x. This value is now determined from the marketmap.
sint32 exponent = 3 [ deprecated = true ];
Comment on lines +21 to +23
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Deprecation of exponent Field Requires Further Updates

The exponent field in proto/dydxprotocol/prices/market_param.proto has been deprecated as intended. However, multiple references to exponent were identified across various parts of the codebase, including but not limited to:

  • protocol/x/vault/keeper/*.go
  • protocol/testutil/*.go
  • protocol/x/revshare/keeper/*.go
  • protocol/x/clob/keeper/*.go
  • protocol/daemons/slinky/client/*.go

This indicates that the deprecation is not fully propagated, and residual dependencies on the exponent field may lead to inconsistencies or legacy issues within the system.

Recommended Actions:

  • Update References: Refactor the identified files to remove or replace usages of the exponent field, ensuring all components rely on the new mechanism provided by the market map.
  • Comprehensive Testing: After updating, perform thorough testing to verify that the removal of exponent does not adversely affect system functionality.
  • Documentation: Ensure that all related documentation and client libraries are updated to reflect the deprecation and the new method of determining the exponent value.
Analysis chain

Deprecation of exponent field looks good, but consider additional documentation.

The deprecation of the exponent field has been correctly implemented using the deprecated = true option. The added comment provides clear information about when the field was deprecated and where the value is now determined.

However, to ensure a smooth transition for users of this protobuf definition, consider the following suggestions:

  1. Add information about how to retrieve the exponent value from the market map.
  2. Specify the version when this field will be removed entirely, if planned.
  3. Update any related documentation or client libraries to reflect this change.

Consider updating the comment to include more details:

  // Static value. The exponent of the price.
  // For example if `Exponent == -5` then a `Value` of `1,000,000,000`
  // represents ``$10,000`. Therefore `10 ^ Exponent` represents the smallest
  // price step (in dollars) that can be recorded.
-  //
-  // Deprecated since v7.1.x. This value is now determined from the marketmap.
+  //
+  // Deprecated since v7.1.x. This value is now determined from the marketmap.
+  // To retrieve the exponent, use [method to get from marketmap].
+  // This field will be removed in v8.0.0.
  sint32 exponent = 3 [ deprecated = true ];

Let's check if there are any other references to exponent in the file that might need updating:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other references to 'exponent' in the file
rg --type protobuf 'exponent' proto/dydxprotocol/prices/market_param.proto

Length of output: 134285


// The minimum number of exchanges that should be reporting a live price for
// a price update to be considered valid.
Expand Down
3 changes: 3 additions & 0 deletions proto/dydxprotocol/prices/market_price.proto
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ message MarketPrice {

// Static value. The exponent of the price. See the comment on the duplicate
// MarketParam field for more information.
//
// As of v7.1.x, this value is determined from the marketmap instead of
// needing to match the MarketParam field.
sint32 exponent = 2;

// The variable value that is updated by oracle price updates. `0` if it has
Expand Down
13 changes: 7 additions & 6 deletions protocol/app/ante/market_update_test.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
package ante_test

import (
storetypes "cosmossdk.io/store/types"
"math/rand"
"testing"

storetypes "cosmossdk.io/store/types"

sdkmath "cosmossdk.io/math"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
"github.com/dydxprotocol/v4-chain/protocol/dtypes"
Expand Down Expand Up @@ -185,7 +186,7 @@ var (
Base: "TESTING",
Quote: "USD",
},
Decimals: 1,
Decimals: 8,
MinProviderCount: 1,
Enabled: false,
Metadata_JSON: "",
Expand All @@ -199,7 +200,7 @@ var (
Base: "TESTING",
Quote: "USD",
},
Decimals: 1,
Decimals: 8,
MinProviderCount: 1,
Enabled: false,
Metadata_JSON: "",
Expand All @@ -218,7 +219,7 @@ var (
Base: "TESTING",
Quote: "USD",
},
Decimals: 1,
Decimals: 8,
MinProviderCount: 1,
Enabled: true,
Metadata_JSON: "",
Expand All @@ -237,7 +238,7 @@ var (
Base: "TESTING",
Quote: "USD",
},
Decimals: 1,
Decimals: 8,
MinProviderCount: 1,
Enabled: true,
Metadata_JSON: "",
Expand Down Expand Up @@ -763,7 +764,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) {
ctx,
mmtypes.Market{
Ticker: mmtypes.Ticker{
Decimals: uint64(pair.market.Exponent),
Decimals: uint64(pair.market.Exponent * -1),
Enabled: false, // will be enabled later
CurrencyPair: cp,
},
Expand Down
28 changes: 28 additions & 0 deletions protocol/mocks/PricesKeeper.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 0 additions & 14 deletions protocol/testing/e2e/gov/prices_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,20 +67,6 @@ func TestUpdateMarketParam(t *testing.T) {
},
expectedProposalStatus: govtypesv1.ProposalStatus_PROPOSAL_STATUS_FAILED,
},
"Failure: exponent is updated": {
msg: &pricestypes.MsgUpdateMarketParam{
Authority: lib.GovModuleAddress.String(),
MarketParam: pricestypes.MarketParam{
Id: MODIFIED_MARKET_PARAM.Id,
Pair: MODIFIED_MARKET_PARAM.Pair,
Exponent: MODIFIED_MARKET_PARAM.Exponent + 1, // update to exponent is not permitted.
MinExchanges: MODIFIED_MARKET_PARAM.MinExchanges,
MinPriceChangePpm: MODIFIED_MARKET_PARAM.MinPriceChangePpm,
ExchangeConfigJson: MODIFIED_MARKET_PARAM.ExchangeConfigJson,
},
},
expectedProposalStatus: govtypesv1.ProposalStatus_PROPOSAL_STATUS_FAILED,
},
"Failure: empty pair": {
msg: &pricestypes.MsgUpdateMarketParam{
Authority: lib.GovModuleAddress.String(),
Expand Down
10 changes: 8 additions & 2 deletions protocol/testutil/app/order.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,17 @@ func MustMakeOrderFromHumanInput(
if !exists {
panic(fmt.Sprintf("marketParam does not exist: %v", perp.Params.MarketId))
}
marketPrice := pricestest.MustHumanPriceToMarketPrice(humanPrice, marketParams.Exponent)

exponent, err := app.PricesKeeper.GetExponent(ctx, marketParams.Pair)
if err != nil {
panic(err)
}

marketPrice := pricestest.MustHumanPriceToMarketPrice(humanPrice, exponent)
subticks := clobtypes.PriceToSubticks(
pricestypes.MarketPrice{
Price: marketPrice,
Exponent: marketParams.Exponent,
Exponent: exponent,
},
clobPair,
perp.Params.AtomicResolution,
Expand Down
12 changes: 6 additions & 6 deletions protocol/testutil/constants/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -4002,7 +4002,7 @@ const GenesisState = `{
},
{
"exchange_config_json": "{\"exchanges\":[{\"exchangeName\":\"Binance\",\"ticker\":\"\\\"LINKUSDT\\\"\"},{\"exchangeName\":\"BinanceUS\",\"ticker\":\"\\\"LINKUSD\\\"\"},{\"exchangeName\":\"CoinbasePro\",\"ticker\":\"LINK-USD\"},{\"exchangeName\":\"CryptoCom\",\"ticker\":\"LINK_USD\"},{\"exchangeName\":\"Huobi\",\"ticker\":\"linkusdt\"},{\"exchangeName\":\"Kraken\",\"ticker\":\"LINKUSD\"},{\"exchangeName\":\"Kucoin\",\"ticker\":\"LINK-USDT\"},{\"exchangeName\":\"Okx\",\"ticker\":\"LINK-USDT\"}]}",
"exponent": -8,
"exponent": -9,
"id": 2,
"min_exchanges": 1,
"min_price_change_ppm": 2000,
Expand Down Expand Up @@ -4170,7 +4170,7 @@ const GenesisState = `{
},
{
"exchange_config_json": "{\"exchanges\":[{\"exchangeName\":\"Binance\",\"ticker\":\"\\\"MKRUSDT\\\"\"},{\"exchangeName\":\"BinanceUS\",\"ticker\":\"\\\"MKRUSD\\\"\"},{\"exchangeName\":\"Bitfinex\",\"ticker\":\"tMKRUSD\"},{\"exchangeName\":\"CoinbasePro\",\"ticker\":\"MKR-USD\"},{\"exchangeName\":\"Gate\",\"ticker\":\"MKR_USDT\"},{\"exchangeName\":\"Huobi\",\"ticker\":\"mkrusdt\"},{\"exchangeName\":\"Kucoin\",\"ticker\":\"MKR-USDT\"},{\"exchangeName\":\"Okx\",\"ticker\":\"MKR-USDT\"}]}",
"exponent": -7,
"exponent": -6,
chenyaoy marked this conversation as resolved.
Show resolved Hide resolved
"id": 23,
"min_exchanges": 1,
"min_price_change_ppm": 2000,
Expand All @@ -4186,7 +4186,7 @@ const GenesisState = `{
},
{
"exchange_config_json": "{\"exchanges\":[{\"exchangeName\":\"Binance\",\"ticker\":\"\\\"XLMUSDT\\\"\"},{\"exchangeName\":\"BinanceUS\",\"ticker\":\"\\\"XLMUSD\\\"\"},{\"exchangeName\":\"Bitfinex\",\"ticker\":\"tXLMUSD\"},{\"exchangeName\":\"CoinbasePro\",\"ticker\":\"XLM-USD\"},{\"exchangeName\":\"Gate\",\"ticker\":\"XLM_USDT\"},{\"exchangeName\":\"Kraken\",\"ticker\":\"XXLMZUSD\"},{\"exchangeName\":\"Kucoin\",\"ticker\":\"XLM-USDT\"},{\"exchangeName\":\"Okx\",\"ticker\":\"XLM-USDT\"}]}",
"exponent": -11,
"exponent": -10,
chenyaoy marked this conversation as resolved.
Show resolved Hide resolved
"id": 25,
"min_exchanges": 1,
"min_price_change_ppm": 2000,
Expand Down Expand Up @@ -4261,7 +4261,7 @@ const GenesisState = `{
"price": 1500000000
},
{
"exponent": -8,
"exponent": -9,
"id": 2,
"price": 700000000
},
Expand Down Expand Up @@ -4366,7 +4366,7 @@ const GenesisState = `{
"price": 2200000000
},
{
"exponent": -7,
"exponent": -6,
"id": 23,
"price": 7100000000
},
Expand All @@ -4376,7 +4376,7 @@ const GenesisState = `{
"price": 7000000000
},
{
"exponent": -11,
"exponent": -10,
"id": 25,
"price": 10000000000
},
Expand Down
30 changes: 28 additions & 2 deletions protocol/x/prices/keeper/market.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,22 @@ func (k Keeper) CreateMarket(
)
}
currencyPairStr := currencyPair.String()
_, err = k.MarketMapKeeper.GetMarket(ctx, currencyPairStr)
marketMapDetails, err := k.MarketMapKeeper.GetMarket(ctx, currencyPairStr)
if err != nil {
return types.MarketParam{}, errorsmod.Wrapf(
types.ErrTickerNotFoundInMarketMap,
currencyPairStr,
)
}

// Check that the exponent of market price is the negation of the decimals value in the market map
if marketPrice.Exponent != int32(marketMapDetails.Ticker.Decimals)*-1 {
return types.MarketParam{}, errorsmod.Wrapf(
types.ErrInvalidMarketPriceExponent,
currencyPairStr,
)
}

paramBytes := k.cdc.MustMarshal(&marketParam)
priceBytes := k.cdc.MustMarshal(&marketPrice)

Expand All @@ -87,7 +95,7 @@ func (k Keeper) CreateMarket(
marketParam.Id,
marketParam.Pair,
marketParam.MinPriceChangePpm,
marketParam.Exponent,
marketPrice.Exponent, // The exponent of the market price is the source of truth, the exponent of the param is deprecated as of v7.1.x
),
),
)
Expand All @@ -111,6 +119,24 @@ func (k Keeper) CreateMarket(
return marketParam, nil
}

// Get the exponent for a market as the negation of the decimals value in the market map
func (k Keeper) GetExponent(ctx sdk.Context, ticker string) (int32, error) {
currencyPair, err := slinky.MarketPairToCurrencyPair(ticker)
if err != nil {
k.Logger(ctx).Error("Could not convert market pair to currency pair", "error", err)
return 0, err
}

marketMapDetails, err := k.MarketMapKeeper.GetMarket(ctx, currencyPair.String())
if err != nil {
return 0, errorsmod.Wrapf(
types.ErrTickerNotFoundInMarketMap,
ticker,
)
}
return int32(marketMapDetails.Ticker.Decimals) * -1, nil
}

// GetAllMarketParamPrices returns a slice of MarketParam, MarketPrice tuples for all markets.
func (k Keeper) GetAllMarketParamPrices(ctx sdk.Context) ([]types.MarketParamPrice, error) {
marketParams := k.GetAllMarketParams(ctx)
Expand Down
4 changes: 0 additions & 4 deletions protocol/x/prices/keeper/market_param.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,6 @@ func (k Keeper) ModifyMarketParam(
}

// Validate update is permitted.
if updatedMarketParam.Exponent != existingParam.Exponent {
return types.MarketParam{},
errorsmod.Wrapf(types.ErrMarketExponentCannotBeUpdated, lib.UintToString(updatedMarketParam.Id))
}
for _, market := range k.GetAllMarketParams(ctx) {
if market.Pair == updatedMarketParam.Pair && market.Id != updatedMarketParam.Id {
return types.MarketParam{}, errorsmod.Wrapf(types.ErrMarketParamPairAlreadyExists, updatedMarketParam.Pair)
Expand Down
28 changes: 6 additions & 22 deletions protocol/x/prices/keeper/market_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,12 @@ func TestCreateMarket_Errors(t *testing.T) {
validExchangeConfigJson := `{"exchanges":[{"exchangeName":"Binance","ticker":"BTCUSDT"}]}`
tests := map[string]struct {
// Setup
pair string
minExchanges uint32
minPriceChangePpm uint32
price uint64
marketPriceIdDoesntMatchMarketParamId bool
marketPriceExponentDoesntMatchMarketParamExponent bool
exchangeConfigJson string
pair string
minExchanges uint32
minPriceChangePpm uint32
price uint64
marketPriceIdDoesntMatchMarketParamId bool
exchangeConfigJson string
// Expected
expectedErr string
}{
Expand Down Expand Up @@ -163,18 +162,6 @@ func TestCreateMarket_Errors(t *testing.T) {
"market param id 1 does not match market price id 2",
).Error(),
},
"Market param and price exponents don't match": {
pair: constants.BtcUsdPair,
minExchanges: uint32(2),
minPriceChangePpm: uint32(50),
price: constants.FiveBillion,
marketPriceExponentDoesntMatchMarketParamExponent: true,
exchangeConfigJson: validExchangeConfigJson,
expectedErr: errorsmod.Wrap(
types.ErrInvalidInput,
"market param 1 exponent -6 does not match market price 1 exponent -5",
).Error(),
},
"Pair already exists": {
pair: "0-0",
minExchanges: uint32(2),
Expand All @@ -201,9 +188,6 @@ func TestCreateMarket_Errors(t *testing.T) {
}

marketPriceExponentOffset := int32(0)
if tc.marketPriceExponentDoesntMatchMarketParamExponent {
marketPriceExponentOffset = int32(1)
}

_, err := keeper.CreateMarket(
ctx,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,17 @@ func (k msgServer) CreateOracleMarket(

ctx := lib.UnwrapSDKContext(goCtx, types.ModuleName)

exponent, err := k.Keeper.GetExponent(ctx, msg.Params.Pair)
if err != nil {
return nil, err
}

// Use zero oracle price to create the new market.
// Note that valid oracle price updates cannot be zero (checked in MsgUpdateMarketPrices.ValidateBasic),
// so a zero oracle price indicates that the oracle price has never been updated.
zeroMarketPrice := types.MarketPrice{
Id: msg.Params.Id,
Exponent: msg.Params.Exponent,
Exponent: exponent,
Price: 0,
}
if _, err = k.Keeper.CreateMarket(ctx, msg.Params, zeroMarketPrice); err != nil {
Expand Down
Loading
Loading