From be03041000279075f4aac2d10d8e482e2a14dd23 Mon Sep 17 00:00:00 2001 From: Nikhil Vasan Date: Tue, 23 Jul 2024 13:58:41 -0700 Subject: [PATCH 01/10] implement ante-handler for rejecting invalid updates --- protocol/app/ante.go | 16 ++-- protocol/app/ante/market_update.go | 95 +++++++++++++++++-- protocol/app/ante/market_update_test.go | 120 ++++++++++++++++++++++-- protocol/app/app.go | 1 + 4 files changed, 212 insertions(+), 20 deletions(-) diff --git a/protocol/app/ante.go b/protocol/app/ante.go index 0178b01432..fda89b3a46 100644 --- a/protocol/app/ante.go +++ b/protocol/app/ante.go @@ -21,6 +21,7 @@ import ( clobtypes "github.com/dydxprotocol/v4-chain/protocol/x/clob/types" perpetualstypes "github.com/dydxprotocol/v4-chain/protocol/x/perpetuals/types" pricestypes "github.com/dydxprotocol/v4-chain/protocol/x/prices/types" + appante "github.com/dydxprotocol/v4-chain/protocol/app/ante" ) // HandlerOptions are the options required for constructing an SDK AnteHandler. @@ -28,12 +29,13 @@ import ( // struct embedding to include the normal cosmos-sdk `HandlerOptions`. type HandlerOptions struct { ante.HandlerOptions - Codec codec.Codec - AuthStoreKey storetypes.StoreKey + Codec codec.Codec + AuthStoreKey storetypes.StoreKey AccountplusKeeper *accountpluskeeper.Keeper - ClobKeeper clobtypes.ClobKeeper - PerpetualsKeeper perpetualstypes.PerpetualsKeeper - PricesKeeper pricestypes.PricesKeeper + ClobKeeper clobtypes.ClobKeeper + PerpetualsKeeper perpetualstypes.PerpetualsKeeper + PricesKeeper pricestypes.PricesKeeper + MarketMapKeeper appante.MarketMapKeeper } // NewAnteHandler returns an AnteHandler that checks and increments sequence @@ -128,7 +130,9 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { sigGasConsume: ante.NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer), clobRateLimit: clobante.NewRateLimitDecorator(options.ClobKeeper), clob: clobante.NewClobDecorator(options.ClobKeeper), - marketUpdates: customante.NewValidateMarketUpdateDecorator(options.PerpetualsKeeper, options.PricesKeeper), + marketUpdates: customante.NewValidateMarketUpdateDecorator( + options.PerpetualsKeeper, options.PricesKeeper, options.MarketMapKeeper, + ), } return h.AnteHandle, nil } diff --git a/protocol/app/ante/market_update.go b/protocol/app/ante/market_update.go index 1f12069070..bebde17403 100644 --- a/protocol/app/ante/market_update.go +++ b/protocol/app/ante/market_update.go @@ -13,14 +13,20 @@ import ( perpetualstypes "github.com/dydxprotocol/v4-chain/protocol/x/perpetuals/types" pricestypes "github.com/dydxprotocol/v4-chain/protocol/x/prices/types" + slinkylibs "github.com/dydxprotocol/v4-chain/protocol/lib/slinky" ) var ErrNoCrossMarketUpdates = errors.New("cannot call MsgUpdateMarkets or MsgUpsertMarkets " + "on a market listed as cross margin") +type MarketMapKeeper interface { + GetAllMarkets(ctx sdk.Context) (map[string]mmtypes.Market, error) +} + type ValidateMarketUpdateDecorator struct { perpKeeper perpetualstypes.PerpetualsKeeper priceKeeper pricestypes.PricesKeeper + marketMapKeeper MarketMapKeeper // write only cache for mapping slinky ticker strings to market types // only evicted on node restart cache map[string]perpetualstypes.PerpetualMarketType @@ -34,10 +40,12 @@ type ValidateMarketUpdateDecorator struct { func NewValidateMarketUpdateDecorator( perpKeeper perpetualstypes.PerpetualsKeeper, priceKeeper pricestypes.PricesKeeper, + marketMapKeeper MarketMapKeeper, ) ValidateMarketUpdateDecorator { return ValidateMarketUpdateDecorator{ perpKeeper: perpKeeper, priceKeeper: priceKeeper, + marketMapKeeper: marketMapKeeper, cache: make(map[string]perpetualstypes.PerpetualMarketType), } } @@ -64,22 +72,29 @@ func (d ValidateMarketUpdateDecorator) AnteHandle( } msgs := tx.GetMsgs() - var msg = msgs[0] + var ( + msg = msgs[0] + markets []mmtypes.Market + ) switch msg := msg.(type) { case *mmtypes.MsgUpdateMarkets: - if contains := d.doMarketsContainCrossMarket(ctx, msg.UpdateMarkets); contains { - return ctx, ErrNoCrossMarketUpdates - } - + markets = msg.UpdateMarkets case *mmtypes.MsgUpsertMarkets: - if contains := d.doMarketsContainCrossMarket(ctx, msg.Markets); contains { - return ctx, ErrNoCrossMarketUpdates - } + markets = msg.Markets default: return ctx, fmt.Errorf("unrecognized message type: %T", msg) } + if contains := d.doMarketsContainCrossMarket(ctx, markets); contains { + return ctx, ErrNoCrossMarketUpdates + } + + // check if the market updates are safe + if err := d.doMarketsUpdateEnabledValues(ctx, markets); err != nil{ + return ctx, errorsmod.Wrap(err, "market update is not safe") + } + return next(ctx, tx, simulate) } @@ -113,6 +128,70 @@ func (d ValidateMarketUpdateDecorator) doMarketsContainCrossMarket(ctx sdk.Conte return false } +// doMarketsUpdateEnabledValues checks if the given markets updates are safe, specifically: +// 1. If a newly added market (market does not exist in x/prices) is added, it should be disabled in the market-map +// 2. If an existing market is updated, the market-update should not change the enabled value +func (d ValidateMarketUpdateDecorator) doMarketsUpdateEnabledValues(ctx sdk.Context, markets []mmtypes.Market) error { + // get all market-params + mps := d.priceKeeper.GetAllMarketParams(ctx) + mm, err := d.marketMapKeeper.GetAllMarkets(ctx) + if err != nil { + return err + } + + // convert to map for easy lookup + mpMap, err := marketParamsSliceToMap(mps) + if err != nil { + return err + } + + // check validity of incoming market-updates + for _, market := range markets { + _, exists := mpMap[market.Ticker.CurrencyPair.String()] + if !exists { + // if market does not exist in x/prices, it should be disabled + if market.Ticker.Enabled { + return errors.New("newly added market should be disabled") + } + } else { + // find the market in the market-map + mmMarket, exists := mm[market.Ticker.CurrencyPair.String()] + if !exists { + return errors.New("market does not exist in market-map") + } + + // if market exists, it should not change the enabled value + if mmMarket.Ticker.Enabled != market.Ticker.Enabled { + return fmt.Errorf("market should not change enabled value from %t to %t", mmMarket.Ticker.Enabled, market.Ticker.Enabled) + } + } + } + + return nil +} + +func marketParamsSliceToMap(mps []pricestypes.MarketParam) (map[string]pricestypes.MarketParam, error) { + mpMap := make(map[string]pricestypes.MarketParam) + + // create entry for each market-param + for _, mp := range mps { + // index will be the slinky-style ticker + idx, err := slinkylibs.MarketPairToCurrencyPair(mp.Pair) + if err != nil { + return nil, err + } + + // check for duplicate entries + if _, exists := mpMap[idx.String()]; exists { + return nil, errors.New("duplicate market-param entry") + } + + mpMap[idx.String()] = mp + } + + return mpMap, nil +} + // IsMarketUpdateTx returns `true` if the supplied `tx` consists of a single // MsgUpdateMarkets or MsgUpsertMarkets func IsMarketUpdateTx(tx sdk.Tx) (bool, error) { diff --git a/protocol/app/ante/market_update_test.go b/protocol/app/ante/market_update_test.go index 9dbd277639..1aba84df27 100644 --- a/protocol/app/ante/market_update_test.go +++ b/protocol/app/ante/market_update_test.go @@ -1,6 +1,9 @@ package ante_test import ( + "math/rand" + "testing" + sdkmath "cosmossdk.io/math" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" "github.com/dydxprotocol/v4-chain/protocol/dtypes" @@ -10,8 +13,6 @@ import ( prices_types "github.com/dydxprotocol/v4-chain/protocol/x/prices/types" "github.com/skip-mev/slinky/pkg/types" mmtypes "github.com/skip-mev/slinky/x/marketmap/types" - "math/rand" - "testing" "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/client/tx" @@ -22,6 +23,7 @@ import ( "github.com/stretchr/testify/require" "github.com/dydxprotocol/v4-chain/protocol/app/ante" + slinkylib "github.com/dydxprotocol/v4-chain/protocol/lib/slinky" testante "github.com/dydxprotocol/v4-chain/protocol/testutil/ante" testapp "github.com/dydxprotocol/v4-chain/protocol/testutil/app" ) @@ -177,6 +179,20 @@ func TestIsMarketUpdateTx(t *testing.T) { var ( testMarket = mmtypes.Market{ + Ticker: mmtypes.Ticker{ + CurrencyPair: types.CurrencyPair{ + Base: "TEST", + Quote: "USD", + }, + Decimals: 1, + MinProviderCount: 1, + Enabled: false, + Metadata_JSON: "", + }, + ProviderConfigs: nil, + } + + testMarketWithEnabled = mmtypes.Market{ Ticker: mmtypes.Ticker{ CurrencyPair: types.CurrencyPair{ Base: "TEST", @@ -210,6 +226,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { msgs []sdk.Msg simulate bool marketPerps []marketPerpPair + marketMapMarkets []mmtypes.Market } tests := []struct { name string @@ -366,8 +383,8 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { &mmtypes.MsgUpsertMarkets{ Authority: constants.BobAccAddress.String(), Markets: []mmtypes.Market{ - testMarket, - }, + testMarketWithEnabled, + }, }, }, simulate: false, @@ -425,18 +442,105 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, wantErr: true, }, + { + name: "reject a single message with a new market that's enabled", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpsertMarkets{ + Authority: constants.BobAccAddress.String(), + Markets: []mmtypes.Market{ + testMarketWithEnabled, + }, + }, + }, + simulate: false, + marketMapMarkets: []mmtypes.Market{testMarket}, + }, + wantErr: true, + }, + { + name: "reject a single message with a new market that's enabled - simulate", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpsertMarkets{ + Authority: constants.BobAccAddress.String(), + Markets: []mmtypes.Market{ + testMarketWithEnabled, + }, + }, + }, + simulate: true, + marketMapMarkets: []mmtypes.Market{testMarket}, + }, + wantErr: true, + }, + { + name: "reject single message disabling a market", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpsertMarkets{ + Authority: constants.BobAccAddress.String(), + Markets: []mmtypes.Market{ + testMarketWithEnabled, + }, + }, + }, + simulate: false, + marketMapMarkets: []mmtypes.Market{testMarket}, + }, + wantErr: true, + }, + { + name: "reject single message disabling a market - simulate", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpsertMarkets{ + Authority: constants.BobAccAddress.String(), + Markets: []mmtypes.Market{ + testMarketWithEnabled, + }, + }, + }, + simulate: true, + marketMapMarkets: []mmtypes.Market{testMarket}, + }, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { tApp := testapp.NewTestAppBuilder(t).Build() ctx := tApp.InitChain() + // setup initial market-map markets + for _, market := range tt.args.marketMapMarkets { + require.NoError(t, tApp.App.MarketMapKeeper.CreateMarket(ctx, market)) + } + // setup initial perps based on test for _, pair := range tt.args.marketPerps { marketID := rand.Uint32() pair.market.Id = marketID - _, err := tApp.App.PricesKeeper.CreateMarket( + cp, err := slinkylib.MarketPairToCurrencyPair(pair.market.Pair) + require.NoError(t, err) + + // create the market in x/marketmap + require.NoError( + t, + tApp.App.MarketMapKeeper.CreateMarket( + ctx, + mmtypes.Market{ + Ticker: mmtypes.Ticker{ + Decimals: uint64(pair.market.Exponent), + Enabled: false, // will be enabled later + CurrencyPair: cp, + }, + }, + ), + ) + + _, err = tApp.App.PricesKeeper.CreateMarket( ctx, pair.market, prices_types.MarketPrice{ @@ -460,7 +564,11 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { require.NoError(t, err) } - wrappedHandler := ante.NewValidateMarketUpdateDecorator(tApp.App.PerpetualsKeeper, tApp.App.PricesKeeper) + wrappedHandler := ante.NewValidateMarketUpdateDecorator( + tApp.App.PerpetualsKeeper, + tApp.App.PricesKeeper, + &tApp.App.MarketMapKeeper, + ) anteHandler := sdk.ChainAnteDecorators(wrappedHandler) // Empty private key, so tx's signature should be empty. diff --git a/protocol/app/app.go b/protocol/app/app.go index 6d23e4209e..fb71a63b9c 100644 --- a/protocol/app/app.go +++ b/protocol/app/app.go @@ -1909,6 +1909,7 @@ func (app *App) buildAnteHandler(txConfig client.TxConfig) sdk.AnteHandler { AuthStoreKey: app.keys[authtypes.StoreKey], PerpetualsKeeper: app.PerpetualsKeeper, PricesKeeper: app.PricesKeeper, + MarketMapKeeper: &app.MarketMapKeeper, }, ) if err != nil { From fcfdb758c21781aa632c4bb739e95e16a4349043 Mon Sep 17 00:00:00 2001 From: Nikhil Vasan Date: Tue, 23 Jul 2024 14:02:50 -0700 Subject: [PATCH 02/10] linting --- protocol/app/ante.go | 3 +-- protocol/app/ante/market_update.go | 16 ++++++++-------- protocol/app/ante/market_update_test.go | 24 ++++++++++++------------ protocol/app/app.go | 1 + 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/protocol/app/ante.go b/protocol/app/ante.go index fda89b3a46..cc2a82d24d 100644 --- a/protocol/app/ante.go +++ b/protocol/app/ante.go @@ -21,7 +21,6 @@ import ( clobtypes "github.com/dydxprotocol/v4-chain/protocol/x/clob/types" perpetualstypes "github.com/dydxprotocol/v4-chain/protocol/x/perpetuals/types" pricestypes "github.com/dydxprotocol/v4-chain/protocol/x/prices/types" - appante "github.com/dydxprotocol/v4-chain/protocol/app/ante" ) // HandlerOptions are the options required for constructing an SDK AnteHandler. @@ -35,7 +34,7 @@ type HandlerOptions struct { ClobKeeper clobtypes.ClobKeeper PerpetualsKeeper perpetualstypes.PerpetualsKeeper PricesKeeper pricestypes.PricesKeeper - MarketMapKeeper appante.MarketMapKeeper + MarketMapKeeper customante.MarketMapKeeper } // NewAnteHandler returns an AnteHandler that checks and increments sequence diff --git a/protocol/app/ante/market_update.go b/protocol/app/ante/market_update.go index bebde17403..431aee9a73 100644 --- a/protocol/app/ante/market_update.go +++ b/protocol/app/ante/market_update.go @@ -11,9 +11,9 @@ import ( slinkytypes "github.com/skip-mev/slinky/pkg/types" mmtypes "github.com/skip-mev/slinky/x/marketmap/types" + slinkylibs "github.com/dydxprotocol/v4-chain/protocol/lib/slinky" perpetualstypes "github.com/dydxprotocol/v4-chain/protocol/x/perpetuals/types" pricestypes "github.com/dydxprotocol/v4-chain/protocol/x/prices/types" - slinkylibs "github.com/dydxprotocol/v4-chain/protocol/lib/slinky" ) var ErrNoCrossMarketUpdates = errors.New("cannot call MsgUpdateMarkets or MsgUpsertMarkets " + @@ -24,8 +24,8 @@ type MarketMapKeeper interface { } type ValidateMarketUpdateDecorator struct { - perpKeeper perpetualstypes.PerpetualsKeeper - priceKeeper pricestypes.PricesKeeper + perpKeeper perpetualstypes.PerpetualsKeeper + priceKeeper pricestypes.PricesKeeper marketMapKeeper MarketMapKeeper // write only cache for mapping slinky ticker strings to market types // only evicted on node restart @@ -43,10 +43,10 @@ func NewValidateMarketUpdateDecorator( marketMapKeeper MarketMapKeeper, ) ValidateMarketUpdateDecorator { return ValidateMarketUpdateDecorator{ - perpKeeper: perpKeeper, - priceKeeper: priceKeeper, + perpKeeper: perpKeeper, + priceKeeper: priceKeeper, marketMapKeeper: marketMapKeeper, - cache: make(map[string]perpetualstypes.PerpetualMarketType), + cache: make(map[string]perpetualstypes.PerpetualMarketType), } } @@ -73,7 +73,7 @@ func (d ValidateMarketUpdateDecorator) AnteHandle( msgs := tx.GetMsgs() var ( - msg = msgs[0] + msg = msgs[0] markets []mmtypes.Market ) @@ -91,7 +91,7 @@ func (d ValidateMarketUpdateDecorator) AnteHandle( } // check if the market updates are safe - if err := d.doMarketsUpdateEnabledValues(ctx, markets); err != nil{ + if err := d.doMarketsUpdateEnabledValues(ctx, markets); err != nil { return ctx, errorsmod.Wrap(err, "market update is not safe") } diff --git a/protocol/app/ante/market_update_test.go b/protocol/app/ante/market_update_test.go index 1aba84df27..8083daa94d 100644 --- a/protocol/app/ante/market_update_test.go +++ b/protocol/app/ante/market_update_test.go @@ -223,9 +223,9 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { } type args struct { - msgs []sdk.Msg - simulate bool - marketPerps []marketPerpPair + msgs []sdk.Msg + simulate bool + marketPerps []marketPerpPair marketMapMarkets []mmtypes.Market } tests := []struct { @@ -384,7 +384,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { Authority: constants.BobAccAddress.String(), Markets: []mmtypes.Market{ testMarketWithEnabled, - }, + }, }, }, simulate: false, @@ -453,7 +453,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, }, }, - simulate: false, + simulate: false, marketMapMarkets: []mmtypes.Market{testMarket}, }, wantErr: true, @@ -469,7 +469,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, }, }, - simulate: true, + simulate: true, marketMapMarkets: []mmtypes.Market{testMarket}, }, wantErr: true, @@ -485,7 +485,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, }, }, - simulate: false, + simulate: false, marketMapMarkets: []mmtypes.Market{testMarket}, }, wantErr: true, @@ -501,7 +501,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, }, }, - simulate: true, + simulate: true, marketMapMarkets: []mmtypes.Market{testMarket}, }, wantErr: true, @@ -527,13 +527,13 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { // create the market in x/marketmap require.NoError( - t, + t, tApp.App.MarketMapKeeper.CreateMarket( ctx, mmtypes.Market{ Ticker: mmtypes.Ticker{ - Decimals: uint64(pair.market.Exponent), - Enabled: false, // will be enabled later + Decimals: uint64(pair.market.Exponent), + Enabled: false, // will be enabled later CurrencyPair: cp, }, }, @@ -565,7 +565,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { } wrappedHandler := ante.NewValidateMarketUpdateDecorator( - tApp.App.PerpetualsKeeper, + tApp.App.PerpetualsKeeper, tApp.App.PricesKeeper, &tApp.App.MarketMapKeeper, ) diff --git a/protocol/app/app.go b/protocol/app/app.go index fb71a63b9c..b7ea942841 100644 --- a/protocol/app/app.go +++ b/protocol/app/app.go @@ -1910,6 +1910,7 @@ func (app *App) buildAnteHandler(txConfig client.TxConfig) sdk.AnteHandler { PerpetualsKeeper: app.PerpetualsKeeper, PricesKeeper: app.PricesKeeper, MarketMapKeeper: &app.MarketMapKeeper, + }, ) if err != nil { From 0d6f9901102098b20d57435560782380e3514ebb Mon Sep 17 00:00:00 2001 From: Nikhil Vasan Date: Wed, 24 Jul 2024 08:35:36 -0700 Subject: [PATCH 03/10] test cases --- protocol/app/ante/market_update_test.go | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/protocol/app/ante/market_update_test.go b/protocol/app/ante/market_update_test.go index 8083daa94d..cde4bbfe0f 100644 --- a/protocol/app/ante/market_update_test.go +++ b/protocol/app/ante/market_update_test.go @@ -227,6 +227,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { simulate bool marketPerps []marketPerpPair marketMapMarkets []mmtypes.Market + marketParams []prices_types.MarketParam } tests := []struct { name string @@ -454,7 +455,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, }, simulate: false, - marketMapMarkets: []mmtypes.Market{testMarket}, + marketMapMarkets: []mmtypes.Market{testMarket}, // existing mm market is disabled }, wantErr: true, }, @@ -470,7 +471,6 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, }, simulate: true, - marketMapMarkets: []mmtypes.Market{testMarket}, }, wantErr: true, }, @@ -481,12 +481,13 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { &mmtypes.MsgUpsertMarkets{ Authority: constants.BobAccAddress.String(), Markets: []mmtypes.Market{ - testMarketWithEnabled, + testMarket, }, }, }, simulate: false, - marketMapMarkets: []mmtypes.Market{testMarket}, + marketMapMarkets: []mmtypes.Market{testMarketWithEnabled}, + marketParams: []prices_types.MarketParam{testMarketParams}, }, wantErr: true, }, @@ -497,12 +498,13 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { &mmtypes.MsgUpsertMarkets{ Authority: constants.BobAccAddress.String(), Markets: []mmtypes.Market{ - testMarketWithEnabled, + testMarket, }, }, }, simulate: true, - marketMapMarkets: []mmtypes.Market{testMarket}, + marketMapMarkets: []mmtypes.Market{testMarketWithEnabled}, + marketParams: []prices_types.MarketParam{testMarketParams}, }, wantErr: true, }, @@ -512,6 +514,17 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { tApp := testapp.NewTestAppBuilder(t).Build() ctx := tApp.InitChain() + for _, mp := range tt.args.marketParams { + marketID := rand.Uint32() + mp.Id = marketID + _, err := tApp.App.PricesKeeper.CreateMarket(ctx, mp, prices_types.MarketPrice{ + Id: marketID, + Exponent: -8, + Price: 10, + }) + require.NoError(t, err) + } + // setup initial market-map markets for _, market := range tt.args.marketMapMarkets { require.NoError(t, tApp.App.MarketMapKeeper.CreateMarket(ctx, market)) From f07ae28f531e039c6d2daca5e1346741d2db6600 Mon Sep 17 00:00:00 2001 From: Nikhil Vasan Date: Wed, 24 Jul 2024 08:44:54 -0700 Subject: [PATCH 04/10] linting --- protocol/app/ante/market_update.go | 5 ++++- protocol/app/ante/market_update_test.go | 8 ++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/protocol/app/ante/market_update.go b/protocol/app/ante/market_update.go index 431aee9a73..c3aa2314d5 100644 --- a/protocol/app/ante/market_update.go +++ b/protocol/app/ante/market_update.go @@ -162,7 +162,10 @@ func (d ValidateMarketUpdateDecorator) doMarketsUpdateEnabledValues(ctx sdk.Cont // if market exists, it should not change the enabled value if mmMarket.Ticker.Enabled != market.Ticker.Enabled { - return fmt.Errorf("market should not change enabled value from %t to %t", mmMarket.Ticker.Enabled, market.Ticker.Enabled) + return fmt.Errorf( + "market should not change enabled value from %t to %t", + mmMarket.Ticker.Enabled, market.Ticker.Enabled, + ) } } } diff --git a/protocol/app/ante/market_update_test.go b/protocol/app/ante/market_update_test.go index cde4bbfe0f..861f98afd6 100644 --- a/protocol/app/ante/market_update_test.go +++ b/protocol/app/ante/market_update_test.go @@ -227,7 +227,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { simulate bool marketPerps []marketPerpPair marketMapMarkets []mmtypes.Market - marketParams []prices_types.MarketParam + marketParams []prices_types.MarketParam } tests := []struct { name string @@ -470,7 +470,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, }, }, - simulate: true, + simulate: true, }, wantErr: true, }, @@ -487,7 +487,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, simulate: false, marketMapMarkets: []mmtypes.Market{testMarketWithEnabled}, - marketParams: []prices_types.MarketParam{testMarketParams}, + marketParams: []prices_types.MarketParam{testMarketParams}, }, wantErr: true, }, @@ -504,7 +504,7 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, simulate: true, marketMapMarkets: []mmtypes.Market{testMarketWithEnabled}, - marketParams: []prices_types.MarketParam{testMarketParams}, + marketParams: []prices_types.MarketParam{testMarketParams}, }, wantErr: true, }, From 6045531afcdc31b72b0fe9379965ec30b21e4b57 Mon Sep 17 00:00:00 2001 From: Nikhil Vasan Date: Wed, 24 Jul 2024 14:32:06 -0700 Subject: [PATCH 05/10] additional test-cases --- protocol/app/ante/market_update.go | 2 +- protocol/app/ante/market_update_test.go | 92 +++++++++++++++++++++++-- 2 files changed, 87 insertions(+), 7 deletions(-) diff --git a/protocol/app/ante/market_update.go b/protocol/app/ante/market_update.go index c3aa2314d5..243dcd8dc2 100644 --- a/protocol/app/ante/market_update.go +++ b/protocol/app/ante/market_update.go @@ -163,7 +163,7 @@ func (d ValidateMarketUpdateDecorator) doMarketsUpdateEnabledValues(ctx sdk.Cont // if market exists, it should not change the enabled value if mmMarket.Ticker.Enabled != market.Ticker.Enabled { return fmt.Errorf( - "market should not change enabled value from %t to %t", + "market should not change enabled value from %t to %t", mmMarket.Ticker.Enabled, market.Ticker.Enabled, ) } diff --git a/protocol/app/ante/market_update_test.go b/protocol/app/ante/market_update_test.go index 861f98afd6..38e948ffa3 100644 --- a/protocol/app/ante/market_update_test.go +++ b/protocol/app/ante/market_update_test.go @@ -459,6 +459,22 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, wantErr: true, }, + { + name: "reject a single update-markets message with a new market that's enabled", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpdateMarkets{ + Authority: constants.BobAccAddress.String(), + UpdateMarkets: []mmtypes.Market{ + testMarketWithEnabled, + }, + }, + }, + simulate: false, + marketMapMarkets: []mmtypes.Market{testMarket}, // existing mm market is disabled + }, + wantErr: true, + }, { name: "reject a single message with a new market that's enabled - simulate", args: args{ @@ -470,7 +486,54 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, }, }, - simulate: true, + simulate: true, + marketMapMarkets: []mmtypes.Market{testMarket}, // existing mm market is disabled + }, + wantErr: true, + }, + { + name: "reject a single message with a new market that's enabled, but doesn't exist in x/marketmap", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpsertMarkets{ + Authority: constants.BobAccAddress.String(), + Markets: []mmtypes.Market{ + testMarketWithEnabled, + }, + }, + }, + simulate: false, + }, + wantErr: true, + }, + { + name: "reject a single update-markets message with a new market that's enabled, but doesn't exist in x/marketmap", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpdateMarkets{ + Authority: constants.BobAccAddress.String(), + UpdateMarkets: []mmtypes.Market{ + testMarketWithEnabled, + }, + }, + }, + simulate: false, + }, + wantErr: true, + }, + { + name: "reject a single message with a new market that's enabled, but doesn't exist in x/marketmap - simulate", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpsertMarkets{ + Authority: constants.BobAccAddress.String(), + Markets: []mmtypes.Market{ + testMarketWithEnabled, + }, + }, + }, + simulate: true, + marketMapMarkets: []mmtypes.Market{testMarket}, // existing mm market is disabled }, wantErr: true, }, @@ -491,6 +554,23 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, wantErr: true, }, + { + name: "reject single update markets message disabling a market", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpsertMarkets{ + Authority: constants.BobAccAddress.String(), + Markets: []mmtypes.Market{ + testMarket, + }, + }, + }, + simulate: false, + marketMapMarkets: []mmtypes.Market{testMarketWithEnabled}, + marketParams: []prices_types.MarketParam{testMarketParams}, + }, + wantErr: true, + }, { name: "reject single message disabling a market - simulate", args: args{ @@ -514,6 +594,11 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { tApp := testapp.NewTestAppBuilder(t).Build() ctx := tApp.InitChain() + // setup initial market-map markets + for _, market := range tt.args.marketMapMarkets { + require.NoError(t, tApp.App.MarketMapKeeper.CreateMarket(ctx, market)) + } + for _, mp := range tt.args.marketParams { marketID := rand.Uint32() mp.Id = marketID @@ -525,11 +610,6 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { require.NoError(t, err) } - // setup initial market-map markets - for _, market := range tt.args.marketMapMarkets { - require.NoError(t, tApp.App.MarketMapKeeper.CreateMarket(ctx, market)) - } - // setup initial perps based on test for _, pair := range tt.args.marketPerps { marketID := rand.Uint32() From c496392d1709f5623a11de66a2957cada39b2a14 Mon Sep 17 00:00:00 2001 From: Nikhil Vasan Date: Wed, 24 Jul 2024 14:52:31 -0700 Subject: [PATCH 06/10] registering error types --- protocol/app/ante.go | 12 ++++++------ protocol/app/ante/market_update.go | 11 +++++------ protocol/app/app.go | 3 +-- protocol/x/prices/types/errors.go | 23 +++++++++++++++++++++++ 4 files changed, 35 insertions(+), 14 deletions(-) diff --git a/protocol/app/ante.go b/protocol/app/ante.go index cc2a82d24d..c4aed0f4a4 100644 --- a/protocol/app/ante.go +++ b/protocol/app/ante.go @@ -28,13 +28,13 @@ import ( // struct embedding to include the normal cosmos-sdk `HandlerOptions`. type HandlerOptions struct { ante.HandlerOptions - Codec codec.Codec - AuthStoreKey storetypes.StoreKey + Codec codec.Codec + AuthStoreKey storetypes.StoreKey AccountplusKeeper *accountpluskeeper.Keeper - ClobKeeper clobtypes.ClobKeeper - PerpetualsKeeper perpetualstypes.PerpetualsKeeper - PricesKeeper pricestypes.PricesKeeper - MarketMapKeeper customante.MarketMapKeeper + ClobKeeper clobtypes.ClobKeeper + PerpetualsKeeper perpetualstypes.PerpetualsKeeper + PricesKeeper pricestypes.PricesKeeper + MarketMapKeeper customante.MarketMapKeeper } // NewAnteHandler returns an AnteHandler that checks and increments sequence diff --git a/protocol/app/ante/market_update.go b/protocol/app/ante/market_update.go index 243dcd8dc2..66c0eca9a6 100644 --- a/protocol/app/ante/market_update.go +++ b/protocol/app/ante/market_update.go @@ -92,7 +92,7 @@ func (d ValidateMarketUpdateDecorator) AnteHandle( // check if the market updates are safe if err := d.doMarketsUpdateEnabledValues(ctx, markets); err != nil { - return ctx, errorsmod.Wrap(err, "market update is not safe") + return ctx, pricestypes.ErrUnsafeMarketUpdate.Wrap(err.Error()) } return next(ctx, tx, simulate) @@ -151,20 +151,19 @@ func (d ValidateMarketUpdateDecorator) doMarketsUpdateEnabledValues(ctx sdk.Cont if !exists { // if market does not exist in x/prices, it should be disabled if market.Ticker.Enabled { - return errors.New("newly added market should be disabled") + return pricestypes.ErrAdditionOfEnabledMarket } } else { // find the market in the market-map mmMarket, exists := mm[market.Ticker.CurrencyPair.String()] if !exists { - return errors.New("market does not exist in market-map") + return pricestypes.ErrMarketDoesNotExistInMarketMap } // if market exists, it should not change the enabled value if mmMarket.Ticker.Enabled != market.Ticker.Enabled { - return fmt.Errorf( - "market should not change enabled value from %t to %t", - mmMarket.Ticker.Enabled, market.Ticker.Enabled, + return pricestypes.ErrMarketUpdateChangesMarketMapEnabledValue.Wrapf( + "market-map market: %t, incoming market update: %t", mmMarket.Ticker.Enabled, market.Ticker.Enabled, ) } } diff --git a/protocol/app/app.go b/protocol/app/app.go index b7ea942841..4938eeb0ba 100644 --- a/protocol/app/app.go +++ b/protocol/app/app.go @@ -1909,8 +1909,7 @@ func (app *App) buildAnteHandler(txConfig client.TxConfig) sdk.AnteHandler { AuthStoreKey: app.keys[authtypes.StoreKey], PerpetualsKeeper: app.PerpetualsKeeper, PricesKeeper: app.PricesKeeper, - MarketMapKeeper: &app.MarketMapKeeper, - + MarketMapKeeper: &app.MarketMapKeeper, }, ) if err != nil { diff --git a/protocol/x/prices/types/errors.go b/protocol/x/prices/types/errors.go index 8b37595bae..db01be0c44 100644 --- a/protocol/x/prices/types/errors.go +++ b/protocol/x/prices/types/errors.go @@ -49,4 +49,27 @@ var ( 500, "Authority is invalid", ) + ErrUnsafeMarketUpdate = errorsmod.Register( + ModuleName, + 501, + "Market update is unsafe", + ) + + ErrMarketUpdateChangesMarketMapEnabledValue = errorsmod.Register( + ModuleName, + 502, + "Market update changes market map enabled value", + ) + + ErrMarketDoesNotExistInMarketMap = errorsmod.Register( + ModuleName, + 503, + "Market does not exist in market map", + ) + + ErrAdditionOfEnabledMarket = errorsmod.Register( + ModuleName, + 504, + "Newly added markets must be disabled", + ) ) From 79d6d298fc6be824e3a619d26332a21574a4dddf Mon Sep 17 00:00:00 2001 From: Nikhil Vasan Date: Thu, 25 Jul 2024 09:59:01 -0700 Subject: [PATCH 07/10] adding additional test-case --- protocol/app/ante/market_update_test.go | 137 ++++++++++++++++++++++++ 1 file changed, 137 insertions(+) diff --git a/protocol/app/ante/market_update_test.go b/protocol/app/ante/market_update_test.go index 38e948ffa3..37540800a3 100644 --- a/protocol/app/ante/market_update_test.go +++ b/protocol/app/ante/market_update_test.go @@ -192,6 +192,44 @@ var ( ProviderConfigs: nil, } + testMarketWithProviderConfig = mmtypes.Market{ + Ticker: mmtypes.Ticker{ + CurrencyPair: types.CurrencyPair{ + Base: "TEST", + Quote: "USD", + }, + Decimals: 1, + MinProviderCount: 1, + Enabled: false, + Metadata_JSON: "", + }, + ProviderConfigs: []mmtypes.ProviderConfig{ + { + Name: "test_provider", + OffChainTicker: "TEST/USD", + }, + }, + } + + enabledTestMarketWithProviderConfig = mmtypes.Market{ + Ticker: mmtypes.Ticker{ + CurrencyPair: types.CurrencyPair{ + Base: "TEST", + Quote: "USD", + }, + Decimals: 1, + MinProviderCount: 1, + Enabled: true, + Metadata_JSON: "", + }, + ProviderConfigs: []mmtypes.ProviderConfig{ + { + Name: "test_provider", + OffChainTicker: "TEST/USD", + }, + }, + } + testMarketWithEnabled = mmtypes.Market{ Ticker: mmtypes.Ticker{ CurrencyPair: types.CurrencyPair{ @@ -588,6 +626,105 @@ func TestValidateMarketUpdateDecorator_AnteHandle(t *testing.T) { }, wantErr: true, }, + { + name: "adding an additional provider, while market is enabled", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpsertMarkets{ + Authority: constants.BobAccAddress.String(), + Markets: []mmtypes.Market{ + enabledTestMarketWithProviderConfig, + }, + }, + }, + simulate: false, + marketMapMarkets: []mmtypes.Market{testMarketWithEnabled}, + marketParams: []prices_types.MarketParam{testMarketParams}, + }, + wantErr: false, + }, + { + name: "adding an additional provider, market is enabled - simulate", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpsertMarkets{ + Authority: constants.BobAccAddress.String(), + Markets: []mmtypes.Market{ + enabledTestMarketWithProviderConfig, + }, + }, + }, + simulate: true, + marketMapMarkets: []mmtypes.Market{testMarketWithEnabled}, + marketParams: []prices_types.MarketParam{testMarketParams}, + }, + wantErr: false, + }, + { + name: "adding an additional provider via update-markets, while market is enabled", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpdateMarkets{ + Authority: constants.BobAccAddress.String(), + UpdateMarkets: []mmtypes.Market{ + enabledTestMarketWithProviderConfig, + }, + }, + }, + simulate: false, + marketMapMarkets: []mmtypes.Market{testMarketWithEnabled}, + marketParams: []prices_types.MarketParam{testMarketParams}, + }, + wantErr: false, + }, + { + name: "adding an additional provider, while market is disabled", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpsertMarkets{ + Authority: constants.BobAccAddress.String(), + Markets: []mmtypes.Market{ + testMarketWithProviderConfig, + }, + }, + }, + simulate: false, + marketMapMarkets: []mmtypes.Market{testMarket}, + }, + wantErr: false, + }, + { + name: "adding an additional provider, while market is disabled - simulate", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpsertMarkets{ + Authority: constants.BobAccAddress.String(), + Markets: []mmtypes.Market{ + testMarketWithProviderConfig, + }, + }, + }, + simulate: true, + marketMapMarkets: []mmtypes.Market{testMarket}, + }, + wantErr: false, + }, + { + name: "adding an additional provider via update-markets, while market is disabled", + args: args{ + msgs: []sdk.Msg{ + &mmtypes.MsgUpdateMarkets{ + Authority: constants.BobAccAddress.String(), + UpdateMarkets: []mmtypes.Market{ + testMarketWithProviderConfig, + }, + }, + }, + simulate: false, + marketMapMarkets: []mmtypes.Market{testMarket}, + }, + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From e2e7f5ca05a2df5af12c2652276be6bb7c6e14ae Mon Sep 17 00:00:00 2001 From: Nikhil Vasan Date: Thu, 25 Jul 2024 11:25:42 -0700 Subject: [PATCH 08/10] rerun CI From a19503b3be25885d3f0b7df754f5ff9e5173af1f Mon Sep 17 00:00:00 2001 From: Nikhil Vasan Date: Thu, 25 Jul 2024 13:53:37 -0700 Subject: [PATCH 09/10] update slinky ref --- protocol/go.mod | 2 +- protocol/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/protocol/go.mod b/protocol/go.mod index 68b01d688c..3e72fe1630 100644 --- a/protocol/go.mod +++ b/protocol/go.mod @@ -67,7 +67,7 @@ require ( github.com/pelletier/go-toml v1.9.5 github.com/rs/zerolog v1.32.0 github.com/shopspring/decimal v1.3.1 - github.com/skip-mev/slinky v1.0.5-0.20240724193421-2d84d381bba4 + github.com/skip-mev/slinky v1.0.5-0.20240724231039-9f85f7f0f7cb github.com/spf13/viper v1.19.0 github.com/syndtr/goleveldb v1.0.1-0.20220721030215-126854af5e6d go.uber.org/zap v1.27.0 diff --git a/protocol/go.sum b/protocol/go.sum index 859129de8b..210340e2ec 100644 --- a/protocol/go.sum +++ b/protocol/go.sum @@ -1754,8 +1754,8 @@ github.com/sivchari/tenv v1.7.1 h1:PSpuD4bu6fSmtWMxSGWcvqUUgIn7k3yOJhOIzVWn8Ak= github.com/sivchari/tenv v1.7.1/go.mod h1:64yStXKSOxDfX47NlhVwND4dHwfZDdbp2Lyl018Icvg= github.com/skip-mev/chaintestutil v0.0.0-20240116134208-3e49bf514803 h1:VRRVYN3wsOIOqVT3e3nDh3vyUl6RvF9QwdK4BvgPP9c= github.com/skip-mev/chaintestutil v0.0.0-20240116134208-3e49bf514803/go.mod h1:LF2koCTmygQnz11yjSfHvNP8axdyZ2lTEw0EwI+dnno= -github.com/skip-mev/slinky v1.0.5-0.20240724193421-2d84d381bba4 h1:WXwV5R+DmBd2dYFOHmh+lXOqCidtIENtDDOz4RKhJ8g= -github.com/skip-mev/slinky v1.0.5-0.20240724193421-2d84d381bba4/go.mod h1:I47xFOLQZ5d0f9cH7k9RtKoakIPLfbT0sJhinOvASBM= +github.com/skip-mev/slinky v1.0.5-0.20240724231039-9f85f7f0f7cb h1:TAjUdgIKTnVg5cXCcrQ6e7Z+lnaT1Fj4SqGbenKo6YI= +github.com/skip-mev/slinky v1.0.5-0.20240724231039-9f85f7f0f7cb/go.mod h1:I47xFOLQZ5d0f9cH7k9RtKoakIPLfbT0sJhinOvASBM= github.com/sonatard/noctx v0.0.2 h1:L7Dz4De2zDQhW8S0t+KUjY0MAQJd6SgVwhzNIc4ok00= github.com/sonatard/noctx v0.0.2/go.mod h1:kzFz+CzWSjQ2OzIm46uJZoXuBpa2+0y3T36U18dWqIo= github.com/sourcegraph/conc v0.3.0 h1:OQTbbt6P72L20UqAkXXuLOj79LfEanQ+YQFNpLA9ySo= From 58d3cc70d4800e9828e3f90623980e917dadee28 Mon Sep 17 00:00:00 2001 From: Nikhil Vasan Date: Thu, 25 Jul 2024 14:13:51 -0700 Subject: [PATCH 10/10] lint --- protocol/app/ante/market_update_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/protocol/app/ante/market_update_test.go b/protocol/app/ante/market_update_test.go index 37540800a3..6ae0e46fb9 100644 --- a/protocol/app/ante/market_update_test.go +++ b/protocol/app/ante/market_update_test.go @@ -181,7 +181,7 @@ var ( testMarket = mmtypes.Market{ Ticker: mmtypes.Ticker{ CurrencyPair: types.CurrencyPair{ - Base: "TEST", + Base: "TESTING", Quote: "USD", }, Decimals: 1, @@ -195,7 +195,7 @@ var ( testMarketWithProviderConfig = mmtypes.Market{ Ticker: mmtypes.Ticker{ CurrencyPair: types.CurrencyPair{ - Base: "TEST", + Base: "TESTING", Quote: "USD", }, Decimals: 1, @@ -214,7 +214,7 @@ var ( enabledTestMarketWithProviderConfig = mmtypes.Market{ Ticker: mmtypes.Ticker{ CurrencyPair: types.CurrencyPair{ - Base: "TEST", + Base: "TESTING", Quote: "USD", }, Decimals: 1, @@ -233,7 +233,7 @@ var ( testMarketWithEnabled = mmtypes.Market{ Ticker: mmtypes.Ticker{ CurrencyPair: types.CurrencyPair{ - Base: "TEST", + Base: "TESTING", Quote: "USD", }, Decimals: 1, @@ -246,7 +246,7 @@ var ( testMarketParams = prices_types.MarketParam{ Id: 0, - Pair: "TEST-USD", + Pair: "TESTING-USD", Exponent: -8, MinExchanges: 1, MinPriceChangePpm: 10,