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

feat: Enforce MarketParam.Pair uniqueness constraint #1193

Merged
merged 7 commits into from
Mar 19, 2024
Merged

feat: Enforce MarketParam.Pair uniqueness constraint #1193

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9dc0311
Enforce MarketParam.Pair uniqueness constraint
Eric-Warehime Mar 18, 2024
c867d60
Fix perps tests
Eric-Warehime Mar 18, 2024
4d7e839
Fix the rest of the tests
Eric-Warehime Mar 18, 2024
0524fc8
Prevent updating MarketParam.Pair
Eric-Warehime Mar 19, 2024
bde69f3
Fix lint
Eric-Warehime Mar 19, 2024
2082e88
Fix prices test
Eric-Warehime Mar 19, 2024
e138330
Allow updates again
Eric-Warehime Mar 19, 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
11 changes: 11 additions & 0 deletions protocol/testutil/keeper/prices.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,17 @@ func AssertMarketEventsNotInIndexerBlock(
require.Equal(t, 0, len(indexerMarketEvents))
}

// AssertNMarketEventsNotInIndexerBlock verifies that N market events were included in the Indexer block message.
func AssertNMarketEventsNotInIndexerBlock(
t *testing.T,
k *keeper.Keeper,
ctx sdk.Context,
n int,
) {
indexerMarketEvents := getMarketEventsFromIndexerBlock(ctx, k)
require.Equal(t, n, len(indexerMarketEvents))
}

// getMarketEventsFromIndexerBlock returns the market events from the Indexer Block event Kafka message.
func getMarketEventsFromIndexerBlock(
ctx sdk.Context,
Expand Down
9 changes: 9 additions & 0 deletions protocol/x/prices/keeper/market.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ func (k Keeper) CreateMarket(
if err := marketPrice.ValidateFromParam(marketParam); err != nil {
return types.MarketParam{}, err
}
// Stateful Validation
for _, market := range k.GetAllMarketParams(ctx) {
if market.Pair == marketParam.Pair {
return types.MarketParam{}, errorsmod.Wrapf(
types.ErrMarketParamPairAlreadyExists,
marketParam.Pair,
)
}
}

paramBytes := k.cdc.MustMarshal(&marketParam)
priceBytes := k.cdc.MustMarshal(&marketPrice)
Expand Down
7 changes: 7 additions & 0 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 @@ -44,6 +44,13 @@ func (k Keeper) ModifyMarketParam(
errorsmod.Wrapf(types.ErrMarketExponentCannotBeUpdated, lib.UintToString(updatedMarketParam.Id))
}

// Validate base/quote uniqueness
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem necessary? Isn't there already a check that the updatedMarket must already exist in state + we've introduced a uniqueness constraint across all market-pairs in state?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this check is actually necessary, as it's possible for a MarketParam update to change the marketParam.Pair, and the updated Pair may conflict with one in state

for _, market := range k.GetAllMarketParams(ctx) {
if market.Pair == updatedMarketParam.Pair && market.Id != updatedMarketParam.Id {
return types.MarketParam{}, errorsmod.Wrapf(types.ErrMarketParamPairAlreadyExists, updatedMarketParam.Pair)
}
}

// Store the modified market param.
marketParamStore := k.getMarketParamStore(ctx)
b := k.cdc.MustMarshal(&updatedMarketParam)
Expand Down
13 changes: 12 additions & 1 deletion protocol/x/prices/keeper/market_param_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,24 @@ func TestModifyMarketParam_Errors(t *testing.T) {
"",
).Error(),
},
"Duplicate pair fails": {
targetId: 0,
pair: "1-1",
minExchanges: uint32(1),
minPriceChangePpm: uint32(50),
exchangeConfigJson: validExchangeConfigJson,
expectedErr: errorsmod.Wrapf(
types.ErrMarketParamPairAlreadyExists,
"1-1",
).Error(),
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
ctx, keeper, _, _, _, mockTimeKeeper := keepertest.PricesKeepers(t)
mockTimeKeeper.On("Now").Return(constants.TimeT)
ctx = ctx.WithTxBytes(constants.TestTxBytes)
keepertest.CreateNMarkets(t, ctx, keeper, 1)
keepertest.CreateNMarkets(t, ctx, keeper, 2)
_, err := keeper.ModifyMarketParam(
ctx,
types.MarketParam{
Expand Down
4 changes: 4 additions & 0 deletions protocol/x/prices/keeper/market_price_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,19 +135,23 @@ func TestGetMarketIdToValidIndexPrice(t *testing.T) {
keeper,
[]types.MarketParamPrice{
*pricestest.GenerateMarketParamPrice(
pricestest.WithPair("0-0"),
pricestest.WithId(6),
pricestest.WithMinExchanges(2),
),
*pricestest.GenerateMarketParamPrice(
pricestest.WithPair("1-1"),
pricestest.WithId(7),
pricestest.WithMinExchanges(2),
),
*pricestest.GenerateMarketParamPrice(
pricestest.WithPair("2-2"),
pricestest.WithId(8),
pricestest.WithMinExchanges(2),
pricestest.WithExponent(-8),
),
*pricestest.GenerateMarketParamPrice(
pricestest.WithPair("3-3"),
pricestest.WithId(9),
pricestest.WithMinExchanges(2),
pricestest.WithExponent(-9),
Expand Down
30 changes: 22 additions & 8 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 @@ -158,7 +158,7 @@ func TestCreateMarket_Errors(t *testing.T) {
exchangeConfigJson: validExchangeConfigJson,
expectedErr: errorsmod.Wrap(
types.ErrInvalidInput,
"market param id 0 does not match market price id 1",
"market param id 1 does not match market price id 2",
).Error(),
},
"Market param and price exponents don't match": {
Expand All @@ -170,15 +170,29 @@ func TestCreateMarket_Errors(t *testing.T) {
exchangeConfigJson: validExchangeConfigJson,
expectedErr: errorsmod.Wrap(
types.ErrInvalidInput,
"market param 0 exponent -6 does not match market price 0 exponent -5",
"market param 1 exponent -6 does not match market price 1 exponent -5",
).Error(),
},
"Pair already exists": {
pair: "0-0",
minExchanges: uint32(2),
minPriceChangePpm: uint32(50),
price: constants.FiveBillion,
exchangeConfigJson: validExchangeConfigJson,
expectedErr: errorsmod.Wrap(
types.ErrMarketParamPairAlreadyExists,
"0-0",
).Error(),
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
ctx, keeper, _, _, _, _ := keepertest.PricesKeepers(t)
ctx, keeper, _, _, _, mockTimeKeeper := keepertest.PricesKeepers(t)
ctx = ctx.WithTxBytes(constants.TestTxBytes)

mockTimeKeeper.On("Now").Return(constants.TimeT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a timekeeper now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other test cases never succesfully create a market param. They all fail before hitting this line

v4-chain/protocol/x/prices/keeper/market.go

Line 64 in ab14ab8

k.marketToCreatedAt[marketParam.Id] = k.timeProvider.Now()

The additional test case relies on a market param already existing so it has to be able to succeed once.

keepertest.CreateNMarkets(t, ctx, keeper, 1)

marketPriceIdOffset := uint32(0)
if tc.marketPriceIdDoesntMatchMarketParamId {
marketPriceIdOffset = uint32(1)
Expand All @@ -192,31 +206,31 @@ func TestCreateMarket_Errors(t *testing.T) {
_, err := keeper.CreateMarket(
ctx,
types.MarketParam{
Id: 0,
Id: 1,
Pair: tc.pair,
Exponent: int32(-6),
MinExchanges: tc.minExchanges,
MinPriceChangePpm: tc.minPriceChangePpm,
ExchangeConfigJson: tc.exchangeConfigJson,
},
types.MarketPrice{
Id: 0 + marketPriceIdOffset,
Id: 1 + marketPriceIdOffset,
Exponent: int32(-6) + marketPriceExponentOffset,
Price: tc.price,
},
)
require.EqualError(t, err, tc.expectedErr)

// Verify no new MarketPrice created.
_, err = keeper.GetMarketPrice(ctx, 0)
_, err = keeper.GetMarketPrice(ctx, 1)
require.EqualError(
t,
err,
errorsmod.Wrap(types.ErrMarketPriceDoesNotExist, lib.UintToString(uint32(0))).Error(),
errorsmod.Wrap(types.ErrMarketPriceDoesNotExist, lib.UintToString(uint32(1))).Error(),
)

// Verify no new market event.
keepertest.AssertMarketEventsNotInIndexerBlock(t, keeper, ctx)
keepertest.AssertNMarketEventsNotInIndexerBlock(t, keeper, ctx, 1)
})
}
}
Expand Down
1 change: 1 addition & 0 deletions protocol/x/prices/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var (
ErrMarketExponentCannotBeUpdated = errorsmod.Register(ModuleName, 202, "Market exponent cannot be updated")
ErrMarketPricesAndParamsDontMatch = errorsmod.Register(ModuleName, 203, "Market prices and params don't match")
ErrMarketParamAlreadyExists = errorsmod.Register(ModuleName, 204, "Market params already exists")
ErrMarketParamPairAlreadyExists = errorsmod.Register(ModuleName, 205, "Market params pair already exists")

// 300 - 399: Price related errors.
ErrIndexPriceNotAvailable = errorsmod.Register(ModuleName, 300, "Index price is not available")
Expand Down
Loading