From 2f407d23ac44c3310909aa257b3c57b4e68ffc8f Mon Sep 17 00:00:00 2001 From: Eric Date: Mon, 9 Sep 2024 21:27:16 -0700 Subject: [PATCH 1/4] Reduce blast radius of errors in MarketMap --- oracle/helpers_test.go | 68 ++++++++ oracle/update.go | 8 +- oracle/update_test.go | 56 ++++++ x/marketmap/types/market.go | 42 +++++ x/marketmap/types/market_test.go | 284 +++++++++++++++++++++++++++++++ 5 files changed, 455 insertions(+), 3 deletions(-) diff --git a/oracle/helpers_test.go b/oracle/helpers_test.go index db57340b8..c544fdaca 100644 --- a/oracle/helpers_test.go +++ b/oracle/helpers_test.go @@ -28,6 +28,8 @@ import ( var ( btcusdtCP = slinkytypes.NewCurrencyPair("BTC", "USDT") + btcusdCP = slinkytypes.NewCurrencyPair("BTC", "USD") + usdtusdCP = slinkytypes.NewCurrencyPair("USDT", "USD") ethusdtCP = slinkytypes.NewCurrencyPair("ETH", "USDT") ) @@ -166,6 +168,72 @@ var ( Type: mmclienttypes.ConfigType, } + validMarketMapSubset = mmtypes.MarketMap{ + Markets: map[string]mmtypes.Market{ + ethusdtCP.String(): { + Ticker: mmtypes.Ticker{ + CurrencyPair: ethusdtCP, + MinProviderCount: 1, + Decimals: 8, + Enabled: true, + }, + ProviderConfigs: []mmtypes.ProviderConfig{ + { + Name: coinbase.Name, + OffChainTicker: coinbaseethusd.GetOffChainTicker(), + }, + { + Name: okx.Name, + OffChainTicker: okxethusd.GetOffChainTicker(), + }, + }, + }, + }, + } + + partialInvalidMarketMap = mmtypes.MarketMap{ + Markets: map[string]mmtypes.Market{ + btcusdCP.String(): { + Ticker: mmtypes.Ticker{ + CurrencyPair: btcusdCP, + MinProviderCount: 1, + Decimals: 8, + Enabled: true, + }, + ProviderConfigs: []mmtypes.ProviderConfig{ + { + Name: coinbase.Name, + OffChainTicker: coinbasebtcusd.GetOffChainTicker(), + NormalizeByPair: &usdtusdCP, + }, + { + Name: okx.Name, + OffChainTicker: okxbtcusd.GetOffChainTicker(), + NormalizeByPair: &usdtusdCP, + }, + }, + }, + ethusdtCP.String(): { + Ticker: mmtypes.Ticker{ + CurrencyPair: ethusdtCP, + MinProviderCount: 1, + Decimals: 8, + Enabled: true, + }, + ProviderConfigs: []mmtypes.ProviderConfig{ + { + Name: coinbase.Name, + OffChainTicker: coinbaseethusd.GetOffChainTicker(), + }, + { + Name: okx.Name, + OffChainTicker: okxethusd.GetOffChainTicker(), + }, + }, + }, + }, + } + // Coinbase and OKX are supported by the marketmap. marketMap = mmtypes.MarketMap{ Markets: map[string]mmtypes.Market{ diff --git a/oracle/update.go b/oracle/update.go index 097a3da56..7f38286a5 100644 --- a/oracle/update.go +++ b/oracle/update.go @@ -19,14 +19,16 @@ func (o *OracleImpl) UpdateMarketMap(marketMap mmtypes.MarketMap) error { o.mut.Lock() defer o.mut.Unlock() - if err := marketMap.ValidateBasic(); err != nil { + validSubset := marketMap.GetValidSubset() + + if err := validSubset.ValidateBasic(); err != nil { o.logger.Error("failed to validate market map", zap.Error(err)) return err } // Iterate over all existing price providers and update their market maps. for name, state := range o.priceProviders { - providerTickers, err := types.ProviderTickersFromMarketMap(name, marketMap) + providerTickers, err := types.ProviderTickersFromMarketMap(name, validSubset) if err != nil { o.logger.Error("failed to create provider market map", zap.String("provider", name), zap.Error(err)) return err @@ -42,7 +44,7 @@ func (o *OracleImpl) UpdateMarketMap(marketMap mmtypes.MarketMap) error { o.priceProviders[name] = updatedState } - o.marketMap = marketMap + o.marketMap = validSubset if o.aggregator != nil { o.aggregator.UpdateMarketMap(o.marketMap) } diff --git a/oracle/update_test.go b/oracle/update_test.go index b19990038..e65ab84ca 100644 --- a/oracle/update_test.go +++ b/oracle/update_test.go @@ -626,4 +626,60 @@ func TestUpdateProviderState(t *testing.T) { 500*time.Millisecond, ) }) + + t.Run("can update the market map with partial failure on NormalizeBy", func(t *testing.T) { + orc, err := oracle.New( + oracleCfg, + noOpPriceAggregator{}, + oracle.WithLogger(logger), + oracle.WithPriceAPIQueryHandlerFactory(oraclefactory.APIQueryHandlerFactory), + oracle.WithPriceWebSocketQueryHandlerFactory(oraclefactory.WebSocketQueryHandlerFactory), + ) + require.NoError(t, err) + o := orc.(*oracle.OracleImpl) + require.NoError(t, o.Init(context.TODO())) + + providers := o.GetProviderState() + require.Len(t, providers, 3) + + // Update the oracle's market map. + require.NoError(t, o.UpdateMarketMap(partialInvalidMarketMap)) + + providers = o.GetProviderState() + + cbTickers, err := types.ProviderTickersFromMarketMap(coinbase.Name, validMarketMapSubset) + require.NoError(t, err) + + // Check the state after the update. + coinbaseState, ok := providers[coinbase.Name] + require.True(t, ok) + checkProviderState( + t, + cbTickers, + coinbase.Name, + providertypes.API, + false, + coinbaseState, + ) + + okxTickers, err := types.ProviderTickersFromMarketMap(okx.Name, validMarketMapSubset) + require.NoError(t, err) + + okxState, ok := providers[okx.Name] + require.True(t, ok) + checkProviderState( + t, + okxTickers, + okx.Name, + providertypes.WebSockets, + false, + okxState, + ) + + binanceState, ok := providers[binance.Name] + require.True(t, ok) + checkProviderState(t, nil, binance.Name, providertypes.API, false, binanceState) + + o.Stop() + }) } diff --git a/x/marketmap/types/market.go b/x/marketmap/types/market.go index 65606f5bc..afccb1235 100644 --- a/x/marketmap/types/market.go +++ b/x/marketmap/types/market.go @@ -40,6 +40,48 @@ func (mm *MarketMap) ValidateBasic() error { return nil } +// GetValidSubset outputs a MarketMap which contains the maximal valid subset of this MarketMap. +// +// In particular, this will eliminate anything which would otherwise cause a failure in ValidateBasic. +// The resulting MarketMap should be able to pass ValidateBasic. +func (mm *MarketMap) GetValidSubset() MarketMap { + validSubset := MarketMap{Markets: make(map[string]Market)} + + // Operates in 2 passes: + // 1. Remove invalid ProviderConfigs + for ticker, market := range mm.Markets { + var validProviderConfigs []ProviderConfig + for _, providerConfig := range market.ProviderConfigs { + if providerConfig.NormalizeByPair != nil { + normalizeMarket, found := mm.Markets[providerConfig.NormalizeByPair.String()] + if !found { + continue + } + + if !normalizeMarket.Ticker.Enabled && market.Ticker.Enabled { + continue + } + } + validProviderConfigs = append(validProviderConfigs, providerConfig) + } + market.ProviderConfigs = validProviderConfigs + validSubset.Markets[ticker] = market + } + // 2. Remove ValidateBasic failures on all included markets + for ticker, market := range validSubset.Markets { + if err := market.ValidateBasic(); err != nil { + delete(validSubset.Markets, ticker) + continue + } + // expect that the ticker (index) is equal to the market.Ticker.String() + if ticker != market.Ticker.String() { + delete(validSubset.Markets, ticker) + continue + } + } + return validSubset +} + // String returns the string representation of the market map. func (mm *MarketMap) String() string { return fmt.Sprintf( diff --git a/x/marketmap/types/market_test.go b/x/marketmap/types/market_test.go index a400c38da..70f4e59c0 100644 --- a/x/marketmap/types/market_test.go +++ b/x/marketmap/types/market_test.go @@ -11,6 +11,8 @@ import ( ) var ( + emptyMM = types.MarketMap{Markets: make(map[string]types.Market)} + btcusdtCP = slinkytypes.NewCurrencyPair("BTC", "USDT") btcusdt = types.Market{ @@ -28,6 +30,22 @@ var ( }, } + btcusdViaUSDC = types.Market{ + Ticker: types.Ticker{ + CurrencyPair: slinkytypes.CurrencyPair{Base: "BTC", Quote: "USD"}, + Decimals: 8, + MinProviderCount: 1, + Enabled: true, + }, + ProviderConfigs: []types.ProviderConfig{ + { + Name: "kucoin", + OffChainTicker: "btc-usdc", + NormalizeByPair: &usdcusd.Ticker.CurrencyPair, + }, + }, + } + btcusd = types.Market{ Ticker: types.Ticker{ CurrencyPair: slinkytypes.CurrencyPair{ @@ -102,6 +120,24 @@ var ( }, } + usdcusdDisabled = types.Market{ + Ticker: types.Ticker{ + CurrencyPair: slinkytypes.CurrencyPair{ + Base: "USDC", + Quote: "USD", + }, + Decimals: 8, + MinProviderCount: 1, + Enabled: false, + }, + ProviderConfigs: []types.ProviderConfig{ + { + Name: "kucoin", + OffChainTicker: "usdc-usd", + }, + }, + } + usdcusd = types.Market{ Ticker: types.Ticker{ CurrencyPair: slinkytypes.CurrencyPair{ @@ -180,8 +216,256 @@ var ( btcusdInvalid.Ticker.String(): btcusdInvalid, usdtusdDisabled.Ticker.String(): usdtusdDisabled, } + + // Entire market removed + partiallyValidMarkets1 = map[string]types.Market{ + // Valid + ethusd.Ticker.String(): ethusd, + usdtusd.Ticker.String(): usdtusd, + // Disabled + usdcusdDisabled.Ticker.String(): usdcusdDisabled, + // Invalid + btcusdViaUSDC.Ticker.String(): btcusdViaUSDC, + } + + validSubset1 = map[string]types.Market{ + // Valid + ethusd.Ticker.String(): ethusd, + usdtusd.Ticker.String(): usdtusd, + usdcusdDisabled.Ticker.String(): usdcusdDisabled, + } + + // Should only remove certain provider configs + partiallyValidMarkets2 = map[string]types.Market{ + btcusd.Ticker.String(): { + Ticker: types.Ticker{ + CurrencyPair: slinkytypes.CurrencyPair{ + Base: "BTC", + Quote: "USD", + }, + Decimals: 8, + MinProviderCount: 1, + Enabled: true, + }, + ProviderConfigs: []types.ProviderConfig{ + { + Name: "kucoin", + OffChainTicker: "btc-usdt", + NormalizeByPair: &usdtusd.Ticker.CurrencyPair, + }, + { + Name: "kucoin", + OffChainTicker: "btc-usdc", + NormalizeByPair: &usdcusd.Ticker.CurrencyPair, + }, + }, + }, + usdtusd.Ticker.String(): usdtusd, + usdcusdDisabled.Ticker.String(): usdcusdDisabled, + } + validSubset2 = map[string]types.Market{ + btcusd.Ticker.String(): btcusd, + usdtusd.Ticker.String(): usdtusd, + usdcusdDisabled.Ticker.String(): usdcusdDisabled, + } ) +func TestMarketMapGetValidSubset(t *testing.T) { + testCases := []struct { + name string + marketMap types.MarketMap + validSubset types.MarketMap + }{ + { + name: "valid empty", + marketMap: types.MarketMap{}, + validSubset: emptyMM, + }, + { + name: "valid map", + marketMap: types.MarketMap{ + Markets: markets, + }, + validSubset: types.MarketMap{ + Markets: markets, + }, + }, + { + name: "invalid disabled normalizeByPair", + marketMap: types.MarketMap{ + Markets: map[string]types.Market{ + usdtusdDisabled.String(): usdtusdDisabled, + }, + }, + validSubset: emptyMM, + }, + { + name: "market with no ticker", + marketMap: types.MarketMap{ + Markets: map[string]types.Market{ + btcusdtCP.String(): { + ProviderConfigs: []types.ProviderConfig{ + { + Name: coinbase.Name, + OffChainTicker: "BTC-USD", + }, + }, + }, + }, + }, + validSubset: emptyMM, + }, + { + name: "empty market", + marketMap: types.MarketMap{ + Markets: map[string]types.Market{ + btcusdtCP.String(): {}, + }, + }, + validSubset: emptyMM, + }, + { + name: "provider config includes a ticker that is not supported", + marketMap: types.MarketMap{ + Markets: map[string]types.Market{ + btcusdtCP.String(): { + Ticker: types.Ticker{ + CurrencyPair: btcusdtCP, + Decimals: 8, + MinProviderCount: 1, + }, + ProviderConfigs: []types.ProviderConfig{ + { + Name: coinbase.Name, + OffChainTicker: "btc-usd", + NormalizeByPair: &slinkytypes.CurrencyPair{Base: "not", Quote: "real"}, + Invert: false, + Metadata_JSON: "", + }, + }, + }, + }, + }, + validSubset: emptyMM, + }, + { + name: "empty provider name", + marketMap: types.MarketMap{ + Markets: map[string]types.Market{ + btcusdtCP.String(): { + Ticker: types.Ticker{ + CurrencyPair: btcusdtCP, + Decimals: 8, + MinProviderCount: 1, + }, + ProviderConfigs: []types.ProviderConfig{ + { + Name: "", + OffChainTicker: "btc-usd", + Invert: false, + Metadata_JSON: "", + }, + }, + }, + }, + }, + validSubset: emptyMM, + }, + { + name: "no provider configs", + marketMap: types.MarketMap{ + Markets: map[string]types.Market{ + btcusdtCP.String(): { + Ticker: types.Ticker{ + CurrencyPair: btcusdtCP, + Decimals: 8, + MinProviderCount: 1, + }, + ProviderConfigs: []types.ProviderConfig{}, + }, + }, + }, + validSubset: emptyMM, + }, + { + name: "market-map with invalid key", + marketMap: types.MarketMap{ + Markets: map[string]types.Market{ + ethusd.String(): { + Ticker: types.Ticker{ + CurrencyPair: btcusdtCP, + Decimals: 8, + MinProviderCount: 1, + }, + ProviderConfigs: []types.ProviderConfig{ + { + Name: coinbase.Name, + OffChainTicker: "BTC-USD", + }, + }, + }, + }, + }, + validSubset: emptyMM, + }, + { + name: "valid single provider", + marketMap: types.MarketMap{ + Markets: map[string]types.Market{ + btcusdtCP.String(): { + Ticker: types.Ticker{ + CurrencyPair: btcusdtCP, + Decimals: 8, + MinProviderCount: 1, + }, + ProviderConfigs: []types.ProviderConfig{ + { + Name: coinbase.Name, + OffChainTicker: "BTC-USD", + }, + }, + }, + }, + }, + validSubset: types.MarketMap{ + Markets: map[string]types.Market{ + btcusdtCP.String(): { + Ticker: types.Ticker{ + CurrencyPair: btcusdtCP, + Decimals: 8, + MinProviderCount: 1, + }, + ProviderConfigs: []types.ProviderConfig{ + { + Name: coinbase.Name, + OffChainTicker: "BTC-USD", + }, + }, + }, + }, + }, + }, + { + name: "invalid disabled normalize, remove entire market", + marketMap: types.MarketMap{Markets: partiallyValidMarkets1}, + validSubset: types.MarketMap{Markets: validSubset1}, + }, + { + name: "invalid disabled normalize, only remove provider config", + marketMap: types.MarketMap{Markets: partiallyValidMarkets2}, + validSubset: types.MarketMap{Markets: validSubset2}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + validSubset := tc.marketMap.GetValidSubset() + require.Equal(t, tc.validSubset, validSubset) + require.NoError(t, validSubset.ValidateBasic()) + }) + } +} + func TestMarketMapValidateBasic(t *testing.T) { testCases := []struct { name string From 77e2262467079236f5e521d95f571299d38f2c67 Mon Sep 17 00:00:00 2001 From: Eric Date: Mon, 9 Sep 2024 21:49:13 -0700 Subject: [PATCH 2/4] Fix test, lint --- oracle/update_test.go | 4 ++-- x/marketmap/types/market_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/oracle/update_test.go b/oracle/update_test.go index e65ab84ca..3dd182b80 100644 --- a/oracle/update_test.go +++ b/oracle/update_test.go @@ -18,7 +18,7 @@ import ( ) func TestUpdateWithMarketMap(t *testing.T) { - t.Run("bad market map is rejected", func(t *testing.T) { + t.Run("bad market map is not rejected", func(t *testing.T) { orc, err := oracle.New( oracleCfg, noOpPriceAggregator{}, @@ -35,7 +35,7 @@ func TestUpdateWithMarketMap(t *testing.T) { "bad": {}, }, }) - require.Error(t, err) + require.NoError(t, err) o.Stop() }) diff --git a/x/marketmap/types/market_test.go b/x/marketmap/types/market_test.go index 70f4e59c0..96f645a9e 100644 --- a/x/marketmap/types/market_test.go +++ b/x/marketmap/types/market_test.go @@ -217,7 +217,7 @@ var ( usdtusdDisabled.Ticker.String(): usdtusdDisabled, } - // Entire market removed + // Entire market removed. partiallyValidMarkets1 = map[string]types.Market{ // Valid ethusd.Ticker.String(): ethusd, @@ -235,7 +235,7 @@ var ( usdcusdDisabled.Ticker.String(): usdcusdDisabled, } - // Should only remove certain provider configs + // Should only remove certain provider configs. partiallyValidMarkets2 = map[string]types.Market{ btcusd.Ticker.String(): { Ticker: types.Ticker{ From 4ae0a6cd9eec8dfb04c99daf5bf62671c976e55e Mon Sep 17 00:00:00 2001 From: Eric Date: Tue, 10 Sep 2024 22:09:37 -0700 Subject: [PATCH 3/4] PR feedback --- oracle/update.go | 9 ++++++--- x/marketmap/types/market.go | 8 ++++++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/oracle/update.go b/oracle/update.go index 7f38286a5..56b74d224 100644 --- a/oracle/update.go +++ b/oracle/update.go @@ -19,13 +19,16 @@ func (o *OracleImpl) UpdateMarketMap(marketMap mmtypes.MarketMap) error { o.mut.Lock() defer o.mut.Unlock() - validSubset := marketMap.GetValidSubset() - - if err := validSubset.ValidateBasic(); err != nil { + validSubset, err := marketMap.GetValidSubset() + if err != nil { o.logger.Error("failed to validate market map", zap.Error(err)) return err } + if len(validSubset.Markets) == 0 { + o.logger.Warn("market map update produced no valid markets to fetch") + } + // Iterate over all existing price providers and update their market maps. for name, state := range o.priceProviders { providerTickers, err := types.ProviderTickersFromMarketMap(name, validSubset) diff --git a/x/marketmap/types/market.go b/x/marketmap/types/market.go index afccb1235..40e1280e0 100644 --- a/x/marketmap/types/market.go +++ b/x/marketmap/types/market.go @@ -44,7 +44,7 @@ func (mm *MarketMap) ValidateBasic() error { // // In particular, this will eliminate anything which would otherwise cause a failure in ValidateBasic. // The resulting MarketMap should be able to pass ValidateBasic. -func (mm *MarketMap) GetValidSubset() MarketMap { +func (mm *MarketMap) GetValidSubset() (MarketMap, error) { validSubset := MarketMap{Markets: make(map[string]Market)} // Operates in 2 passes: @@ -79,7 +79,11 @@ func (mm *MarketMap) GetValidSubset() MarketMap { continue } } - return validSubset + if valErr := validSubset.ValidateBasic(); valErr != nil { + return validSubset, valErr + } + + return validSubset, nil } // String returns the string representation of the market map. From 58a5c5a368a5726c3c0095b59aa233a3d13b3165 Mon Sep 17 00:00:00 2001 From: Eric Date: Wed, 11 Sep 2024 08:26:15 -0700 Subject: [PATCH 4/4] Fix return val --- x/marketmap/types/market_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/marketmap/types/market_test.go b/x/marketmap/types/market_test.go index 96f645a9e..9b9bcbd71 100644 --- a/x/marketmap/types/market_test.go +++ b/x/marketmap/types/market_test.go @@ -459,9 +459,9 @@ func TestMarketMapGetValidSubset(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - validSubset := tc.marketMap.GetValidSubset() + validSubset, err := tc.marketMap.GetValidSubset() require.Equal(t, tc.validSubset, validSubset) - require.NoError(t, validSubset.ValidateBasic()) + require.NoError(t, err) }) } }