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.

Sign up for GitHub

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

Jump to bottom

[TRA-611] Get MarketPrice exponent from marketmap #2324

Merged
merged 14 commits into from
Oct 1, 2024
Merged

[TRA-611] Get MarketPrice exponent from marketmap #2324

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
49fd999
get exponent from marketmap
chenyaoy Sep 19, 2024
65cb01b
undo
chenyaoy Sep 19, 2024
7f9e572
fix tests
chenyaoy Sep 23, 2024
0859b2d
fix tests
chenyaoy Sep 23, 2024
c9eb62e
indexer proto
chenyaoy Sep 23, 2024
0cece3f
lint
chenyaoy Sep 23, 2024
6dcd8fc
fix simulation test
chenyaoy Sep 23, 2024
b17bdc7
add market price exponent validation test
chenyaoy Sep 24, 2024
6160cf5
add msg server create oracle market test for exponent change
chenyaoy Sep 24, 2024
f48bae2
Merge branch 'main' into chenyao/get-exponent-from-marketmap
chenyaoy Sep 24, 2024
7352992
fix
chenyaoy Sep 24, 2024
48b82f6
missed one
chenyaoy Oct 1, 2024
4a484fc
continue instead of returning nil
chenyaoy Oct 1, 2024
cc33da3
Merge branch 'main' into chenyao/get-exponent-from-marketmap
chenyaoy Oct 1, 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
8 changes: 8 additions & 0 deletions indexer/packages/v4-protos/src/codegen/dydxprotocol/prices/market_param.ts
View file
Open in desktop
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
6 changes: 6 additions & 0 deletions indexer/packages/v4-protos/src/codegen/dydxprotocol/prices/market_price.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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,
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistency Found in LinkUsdExponent Value

The LinkUsdExponent in protocol/testutil/constants/prices.go remains set to -8, which does not align with the updated exponent -9 in genesis.go. This discrepancy may lead to inconsistent price precision for LINK-USD across the system.

🔗 Analysis chain

Verify the intentionality and impact of increased LINK-USD price precision

The exponent for the LINK-USD market (id: 2) has been changed from -8 to -9, increasing the price precision by one decimal place.

Please confirm if this change is intentional and consistent with other parts of the system. Let's verify its impact:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any hardcoded uses of the old exponent (-8) for LINK-USD
rg -i 'link.*usd.*-8' --type go

# Check for any price-related calculations specific to LINK that might be affected
rg -i 'link.*usd.*price' --type go

# Look for any other occurrences of LINK-USD market ID (2) that might need updating
rg -i 'market.*id.*2.*link' --type go

Length of output: 798

"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
31 changes: 29 additions & 2 deletions protocol/x/prices/keeper/market.go
View file
Open in desktop
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,8 @@ func (k Keeper) CreateMarket(
marketParam.Id,
marketParam.Pair,
marketParam.MinPriceChangePpm,
marketParam.Exponent,
// The exponent in market price is the source of truth, the exponent of the param is deprecated as of v7.1.x
marketPrice.Exponent,
),
),
)
Expand All @@ -111,6 +120,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
View file
Open in desktop
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
View file
Open in desktop
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
7 changes: 6 additions & 1 deletion protocol/x/prices/keeper/msg_server_create_oracle_market.go
View file
Open in desktop
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