fix(feeder): improve price provider error handling with graceful degradation, remove panics from all price provider constructors, add registry for sources (#87)

* fix(robustness): remove panic paths from all price provider constructors, make invalid price values consistent, and remove switch case maintenance burden using a registry

* linter

* refactor: add suggestions from coderabbitai

* lots of docs as a finishing touch

Author: Unique-Divine

config/config.go

@@ -31,7 +31,7 @@ var defaultExchangeSymbolsMap = map[string]map[asset.Pair]types.Symbol{
 	// },
 
 	// https://api-pub.bitfinex.com/v2/conf/pub:list:pair:exchange
-	sources.SourceBitfinex: {
+	sources.SourceNameBitfinex: {
 		"ubtc:uusd":  "tBTCUSD",
 		"ueth:uusd":  "tETHUSD",
 		"uusdc:uusd": "tUDCUSD",
@@ -41,7 +41,7 @@ var defaultExchangeSymbolsMap = map[string]map[asset.Pair]types.Symbol{
 	},
 
 	// https://api.gateio.ws/api/v4/spot/currency_pairs
-	sources.SourceGateIo: {
+	sources.SourceNameGateIo: {
 		"ubtc:uusd":  "BTC_USDT",
 		"ueth:uusd":  "ETH_USDT",
 		"uusdc:uusd": "USDC_USDT",
@@ -52,7 +52,7 @@ var defaultExchangeSymbolsMap = map[string]map[asset.Pair]types.Symbol{
 	},
 
 	// https://www.okx.com/api/v5/market/tickers?instType=SPOT
-	sources.SourceOkex: {
+	sources.SourceNameOkex: {
 		"ubtc:uusd":  "BTC-USDT",
 		"ueth:uusd":  "ETH-USDT",
 		"uusdc:uusd": "USDC-USDT",
@@ -62,7 +62,7 @@ var defaultExchangeSymbolsMap = map[string]map[asset.Pair]types.Symbol{
 	},
 
 	// https://api.bybit.com/v5/market/tickers?category=spot
-	sources.SourceBybit: {
+	sources.SourceNameBybit: {
 		"ubtc:uusd":  "BTCUSDT",
 		"ueth:uusd":  "ETHUSDT",
 		"uusdc:uusd": "USDCUSDT",
@@ -71,19 +71,19 @@ var defaultExchangeSymbolsMap = map[string]map[asset.Pair]types.Symbol{
 		"usol:uusd":  "SOLUSDT",
 	},
 
-	sources.SourceErisProtocol: {
+	sources.SourceNameErisProtocol: {
 		"ustnibi:unibi": "ustnibi:unibi", // this is the only pair supported by the Eris Protocol smart contract
 	},
 
-	sources.SourceUniswapV3: {
+	sources.SourceNameUniswapV3: {
 		"usda:usd": "USDa:USDT",
 	},
 
-	sources.SourceChainLink: {
+	sources.SourceNameChainLink: {
 		"b2btc:btc": "uBTC/BTC",
 	},
 
-	sources.SourceAvalon: {
+	sources.SourceNameAvalon: {
 		"susda:usda": "susda:usda",
 	},
 }

feeder/feeder.go

@@ -100,7 +100,7 @@ func (f *Feeder) handleVotingPeriod(vp types.VotingPeriod) {
 		price := f.PriceProvider.GetPrice(p)
 		if !price.Valid {
 			f.logger.Err(fmt.Errorf("no valid price")).Str("asset", p.String()).Str("source", price.SourceName)
-			price.Price = 0
+			price.Price = types.PriceAbstain
 		}
 		prices[i] = price
 	}

feeder/feeder_test.go

@@ -27,7 +27,10 @@ func TestRunPanics(t *testing.T) {
 
 	require.Panics(t, func() {
 		f.Run()
-	})
+	}, `expect panic on Run because the mock ParamsUpdate returns an unbuffered 
+		channel. This means that when initParamsOrDie tries to receive from it, 
+		the channel will be empty, and the receive blocks. This means the "Run" 
+		will timeout after InitTimeout.`)
 }
 
 func TestParamsUpdate(t *testing.T) {
@@ -62,7 +65,7 @@ func TestVotingPeriod(t *testing.T) {
 	}
 
 	abstainPrice := invalidPrice
-	abstainPrice.Price = 0.0
+	abstainPrice.Price = types.PriceAbstain
 
 	tf.mockPriceProvider.EXPECT().GetPrice(asset.Registry.Pair(denoms.BTC, denoms.NUSD)).Return(validPrice)
 	tf.mockPriceProvider.EXPECT().GetPrice(asset.Registry.Pair(denoms.ETH, denoms.NUSD)).Return(invalidPrice)

feeder/integration_test.go

@@ -91,7 +91,7 @@ func (s *IntegrationSuite) SetupSuite() {
 			logger,
 		),
 		feeder.NewPriceProvider(
-			sources.SourceBitfinex,
+			sources.SourceNameBitfinex,
 			map[asset.Pair]types.Symbol{
 				asset.Registry.Pair(denoms.BTC, denoms.NUSD): "tBTCUSD",
 				asset.Registry.Pair(denoms.ETH, denoms.NUSD): "tETHUSD",

feeder/priceposter.go

@@ -169,6 +169,13 @@ func TryUntilDone(
 /// SIGNING
 /// -------------------------------------------------
 
+// sendTx constructs, signs, and broadcasts a transaction containing the provided
+// messages. It retrieves account information (account number and sequence) from
+// the chain and sets transaction fees and gas limits. Returns the transaction
+// response or an error if the broadcast fails or the transaction is rejected.
+//
+// Panics on configuration errors (missing key, encoding failures) as these
+// indicate programmer errors rather than runtime failures.
 func sendTx(
 	ctx context.Context,
 	keyBase keyring.Keyring,
@@ -233,7 +240,15 @@ func sendTx(
 	return resp.TxResponse, nil
 }
 
-func getAccount(ctx context.Context, authClient Auth, ir codectypes.InterfaceRegistry, feeder sdk.AccAddress) (uint64, uint64, error) {
+// The [getAccount] fn retrieves the account number and sequence for the given feeder
+// address from the chain. These values are required for constructing valid
+// transactions. Returns an error if the account cannot be found or unpacked.
+func getAccount(
+	ctx context.Context,
+	authClient Auth,
+	ir codectypes.InterfaceRegistry,
+	feeder sdk.AccAddress,
+) (uint64, uint64, error) {
 	accRaw, err := authClient.Account(ctx, &authtypes.QueryAccountRequest{Address: feeder.String()})
 	if err != nil {
 		return 0, 0, err // if account not found it's pointless to continue
@@ -257,6 +272,13 @@ func getAccount(ctx context.Context, authClient Auth, ir codectypes.InterfaceReg
 // TODO(mercilex): if we used digits + alphanumerics it's more randomized
 var MaxSaltNumber = big.NewInt(9999) // NOTE(mercilex): max salt length is 4
 
+// The [vote] fn submits a prevote message to the chain, and optionally a vote message
+// if an old prevote exists. The vote is constructed from the old prevote's
+// hash and salt. Messages are ordered such that the vote (if present) is sent
+// before the new prevote, as the new prevote will overwrite the old one.
+//
+// oldPrevote may be nil if no previous prevote exists, in which case only
+// the new prevote is submitted.
 func vote(
 	ctx context.Context,
 	newPrevote, oldPrevote *prevote,
@@ -290,6 +312,11 @@ func vote(
 	)
 }
 
+// The [prepareVote] fn constructs a vote message from an existing prevote on the chain.
+// It verifies that the local prevote hash matches the chain's prevote hash
+// to ensure the prevote hasn't been tampered with or expired. Returns nil
+// if no prevote exists on the chain or if the hashes don't match, indicating
+// the prevote has expired or been invalidated.
 func prepareVote(
 	ctx context.Context,
 	oracleClient Oracle,
@@ -323,12 +350,20 @@ func prepareVote(
 	}, nil
 }
 
+// The [prevote] struct contains the data needed for a prevote-vote cycle in the
+// oracle. The [prevote] message is sent first with a hash of the vote data,
+// followed by the actual vote message in the next voting period. This two-phase
+// commit pattern prevents front-running and ensures vote integrity.
 type prevote struct {
 	msg  *oracletypes.MsgAggregateExchangeRatePrevote
 	salt string
 	vote string
 }
 
+// The [newPrevote] fn creates a new prevote message from a list of prices. It generates
+// a random salt value, formats the prices as exchange rate tuples, and computes
+// the aggregate vote hash. The prevote struct contains both the message to send
+// and the data needed to construct the corresponding vote in the next voting period.
 func newPrevote(prices []types.Price, validator sdk.ValAddress, feeder sdk.AccAddress) *prevote {
 	tuple := make(oracletypes.ExchangeRateTuples, len(prices))
 	for i, price := range prices {

feeder/priceprovider.go

@@ -2,6 +2,7 @@ package feeder
 
 import (
 	"encoding/json"
+	"fmt"
 	"sync"
 	"time"
 
@@ -55,31 +56,12 @@ func NewPriceProvider(
 		symbols.Add(s)
 	}
 
-	switch sourceName {
-	case sources.SourceBitfinex:
-		source = sources.NewTickSource(symbols, sources.BitfinexPriceUpdate, logger)
-	case sources.SourceBinance:
-		source = sources.NewTickSource(symbols, sources.BinancePriceUpdate, logger)
-	case sources.SourceCoingecko:
-		source = sources.NewTickSource(symbols, sources.CoingeckoPriceUpdate(config), logger)
-	case sources.SourceOkex:
-		source = sources.NewTickSource(symbols, sources.OkexPriceUpdate, logger)
-	case sources.SourceGateIo:
-		source = sources.NewTickSource(symbols, sources.GateIoPriceUpdate, logger)
-	case sources.SourceCoinMarketCap:
-		source = sources.NewTickSource(symbols, sources.CoinmarketcapPriceUpdate(config), logger)
-	case sources.SourceBybit:
-		source = sources.NewTickSource(symbols, sources.BybitPriceUpdate, logger)
-	case sources.SourceErisProtocol:
-		source = sources.NewTickSource(symbols, sources.ErisProtocolPriceUpdate, logger)
-	case sources.SourceUniswapV3:
-		source = sources.NewTickSource(symbols, sources.UniswapV3PriceUpdate, logger)
-	case sources.SourceAvalon:
-		source = sources.NewTickSource(symbols, sources.AvalonPriceUpdate, logger)
-	case sources.SourceChainLink:
-		source = sources.NewTickSource(symbols, sources.ChainlinkPriceUpdate, logger)
-	default:
-		panic("unknown price provider: " + sourceName)
+	source, err := sources.GetRegisteredSource(sourceName, symbols, config, logger)
+	if err != nil {
+		logger.
+			Warn().
+			Msg(err.Error())
+		return types.NullPriceProvider{}
 	}
 
 	return newPriceProvider(source, sourceName, pairToSymbolMap, logger)
@@ -105,6 +87,10 @@ func newPriceProvider(source types.Source, sourceName string, pairToSymbolsMap m
 	return pp
 }
 
+// loop runs in a background goroutine and continuously listens for price updates
+// from the source. It updates the lastPrices map with new data and handles
+// shutdown signals. The loop exits when stopSignal is closed, ensuring proper
+// cleanup of the source and done channel.
 func (p *PriceProvider) loop() {
 	defer close(p.done)
 	defer p.source.Close()
@@ -135,7 +121,7 @@ func (p *PriceProvider) GetPrice(pair asset.Pair) types.Price {
 		p.logger.Debug().Str("pair", pair.String()).Msg("pair not configured for this pricefeeder")
 		return types.Price{
 			Pair:       pair,
-			Price:      -1, // abstain
+			Price:      types.PriceAbstain,
 			SourceName: p.sourceName,
 			Valid:      false,
 		}
@@ -158,8 +144,9 @@ func (p *PriceProvider) Close() {
 	<-p.done
 }
 
-// isValid is a helper function which asserts if a price is valid given
-// if it was found and the time at which it was last updated.
+// isValid determines whether a price is valid based on whether it was found and
+// whether it was updated within the [types.PriceTimeout] window. Prices that are
+// missing or older than [types.PriceTimeout] are considered invalid.
 func isValid(price types.RawPrice, found bool) bool {
 	return found && time.Since(price.UpdateTime) < types.PriceTimeout
 }
@@ -171,8 +158,11 @@ func isValid(price types.RawPrice, found bool) bool {
 // AggregatePriceProvider aggregates multiple price providers
 // and queries them for prices.
 type AggregatePriceProvider struct {
+	// providers: A set of providers implemented using a map to zero size
+	// empty struct. Using a map gives us random order of iteration, the
+	// intended behavior (since golang's map range is unordered)
+	providers map[types.PriceProvider]struct{}
 	logger    zerolog.Logger
-	providers map[int]types.PriceProvider // we use a map here to provide random ranging (since golang's map range is unordered)
 }
 
 // NewAggregatePriceProvider instantiates a new AggregatePriceProvider instance
@@ -182,11 +172,24 @@ func NewAggregatePriceProvider(
 	sourceConfigMap map[string]json.RawMessage,
 	logger zerolog.Logger,
 ) types.PriceProvider {
-	providers := make(map[int]types.PriceProvider, len(sourcesToPairSymbolMap))
-	i := 0
+	providers := make(map[types.PriceProvider]struct{})
+	invalidSources := []string{}
 	for sourceName, pairToSymbolMap := range sourcesToPairSymbolMap {
-		providers[i] = NewPriceProvider(sourceName, pairToSymbolMap, sourceConfigMap[sourceName], logger)
-		i++
+		pp := NewPriceProvider(sourceName, pairToSymbolMap, sourceConfigMap[sourceName], logger)
+		if _, isNull := pp.(types.NullPriceProvider); isNull {
+			invalidSources = append(invalidSources, sourceName)
+			continue
+		}
+		providers[pp] = struct{}{}
+	}
+
+	if len(providers) != len(sourcesToPairSymbolMap) {
+		logger.Warn().
+			Msg(fmt.Sprintf("invalid source names given as key in configuration: { invalidSources: %#v }", invalidSources))
+	}
+	if len(providers) == 0 {
+		logger.Error().
+			Msg(fmt.Sprintf("no price providers available: { invalidSources: %#v }", invalidSources))
 	}
 
 	return AggregatePriceProvider{
@@ -210,8 +213,8 @@ func (a AggregatePriceProvider) GetPrice(pair asset.Pair) types.Price {
 	case "ustnibi:uusd":
 		// fetch unibi:uusd first to calculate the ustnibi:unibi price
 
-		unibiUusdPrice := -1.0 // default to -1 to indicate we haven't found a valid price yet
-		for _, p := range a.providers {
+		unibiUusdPrice := types.PriceAbstain // default to -1 to indicate we haven't found a valid price yet
+		for p := range a.providers {
 			price := p.GetPrice("unibi:uusd")
 			if !price.Valid {
 				continue
@@ -228,13 +231,13 @@ func (a AggregatePriceProvider) GetPrice(pair asset.Pair) types.Price {
 			return types.Price{
 				SourceName: "missing",
 				Pair:       pair,
-				Price:      0,
+				Price:      types.PriceAbstain,
 				Valid:      false,
 			}
 		}
 
 		// now we can calculate the ustnibi:unibi price
-		for _, p := range a.providers {
+		for p := range a.providers {
 			price := p.GetPrice("ustnibi:unibi")
 			if !price.Valid {
 				continue
@@ -257,7 +260,7 @@ func (a AggregatePriceProvider) GetPrice(pair asset.Pair) types.Price {
 			return types.Price{
 				SourceName: "missing",
 				Pair:       pair,
-				Price:      0,
+				Price:      types.PriceAbstain,
 				Valid:      false,
 			}
 		}
@@ -278,14 +281,14 @@ func (a AggregatePriceProvider) GetPrice(pair asset.Pair) types.Price {
 		return types.Price{
 			SourceName: "missing",
 			Pair:       pair,
-			Price:      0,
+			Price:      types.PriceAbstain,
 			Valid:      false,
 		}
 
 	default:
 		// for all other price pairs, iterate randomly, if we find a valid price, we return it
 		// otherwise we go onto the next PriceProvider to ask for prices.
-		for _, p := range a.providers {
+		for p := range a.providers {
 			price := p.GetPrice(pair)
 			if price.Valid {
 				aggregatePriceProvider.WithLabelValues(pair.String(), price.SourceName, "true").Inc()
@@ -300,13 +303,13 @@ func (a AggregatePriceProvider) GetPrice(pair asset.Pair) types.Price {
 	return types.Price{
 		SourceName: "missing",
 		Pair:       pair,
-		Price:      0,
+		Price:      types.PriceAbstain,
 		Valid:      false,
 	}
 }
 
 func (a AggregatePriceProvider) Close() {
-	for _, p := range a.providers {
+	for p := range a.providers {
 		p.Close()
 	}
 }