changes based on chappjc review

client/core/core.go

@@ -1278,17 +1278,14 @@ func (c *Core) Run(ctx context.Context) {
 		}
 
 		// Construct enabled fiat rate sources.
+	fetchers:
 		for token, rateFetcher := range fiatRateFetchers {
-			var isDisabled bool
 			for _, v := range disabledSources {
 				if token == v {
-					isDisabled = true
-					break
+					continue fetchers
 				}
 			}
-			if !isDisabled {
-				c.fiatRateSources[token] = newCommonRateSource(rateFetcher)
-			}
+			c.fiatRateSources[token] = newCommonRateSource(rateFetcher)
 		}
 
 		// Start goroutine for fiat rate fetcher's if we have at least one source.
@@ -7776,73 +7773,55 @@ func (c *Core) fetchFiatExchangeRates() {
 	ctx, cancel := context.WithCancel(c.ctx)
 	c.stopFiatRateFetching = cancel
 
+	c.log.Debug("starting fiat rate fetching")
+
 	c.wg.Add(1)
 	go func() {
 		defer c.wg.Done()
-		tick := time.NewTimer(0) // starts rate fetching immediately.
+		tick := time.NewTimer(fiatRateRequestInterval)
+		defer tick.Stop()
 		for {
+
+			c.refreshFiatRates(ctx)
+
 			select {
 			case <-ctx.Done():
-				tick.Stop()
 				return
 			case <-tick.C:
-				c.refreshFiatRates(ctx)
 			}
-			tick = c.nextRateSourceTick()
 		}
 	}()
 }
 
 // refreshFiatRates refreshes the fiat rates for rate sources whose values have
-// not been updated since fiatRateRequestInterval.
+// not been updated since fiatRateRequestInterval. It also checks if fiat rates
+// are expired and does some clean-up.
 func (c *Core) refreshFiatRates(ctx context.Context) {
 	ctx, cancel := context.WithTimeout(ctx, 4*time.Second)
 	defer cancel()
 
 	var wg sync.WaitGroup
 	supportedAssets := c.SupportedAssets()
-	timeNow := time.Now()
-	c.ratesMtx.Lock()
+	c.ratesMtx.RLock()
 	for _, source := range c.fiatRateSources {
-		if time.Since(source.lastRequest) > fiatRateRequestInterval {
-			wg.Add(1)
-			go func(source *commonRateSource) {
-				source.lastRequest = timeNow
-				defer wg.Done()
-				source.refreshRates(ctx, c.log, supportedAssets)
-			}(source)
-		}
+		wg.Add(1)
+		go func(source *commonRateSource) {
+			defer wg.Done()
+			source.refreshRates(ctx, c.log, supportedAssets)
+		}(source)
 	}
-	c.ratesMtx.Unlock()
+	c.ratesMtx.RUnlock()
 	wg.Wait()
 
+	// Remove expired rate source if any.
+	c.removeExpiredRateSources()
+
 	fiatRatesMap := c.fiatConversions()
 	if len(fiatRatesMap) != 0 {
 		c.notify(newFiatRatesUpdate(fiatRatesMap))
 	}
 }
 
-// nextRateSourceTick checks the fiat rate source's last request, and calculates
-// when the next cycle should run.
-func (c *Core) nextRateSourceTick() *time.Timer {
-	c.ratesMtx.RLock()
-	defer c.ratesMtx.RUnlock()
-
-	minTick := 10 * time.Second
-	tOldest := time.Now()
-	for _, source := range c.fiatRateSources {
-		t := source.lastRequest
-		if t.Before(tOldest) {
-			tOldest = t
-		}
-	}
-	tilNext := fiatRateRequestInterval - time.Since(tOldest)
-	if tilNext < minTick {
-		tilNext = minTick
-	}
-	return time.NewTimer(tilNext)
-}
-
 // FiatRateSources returns a list of fiat rate sources and their individual
 // status.
 func (c *Core) FiatRateSources() map[string]bool {
@@ -7856,24 +7835,17 @@ func (c *Core) FiatRateSources() map[string]bool {
 }
 
 // fiatConversions returns fiat rate for all supported assets that have a
-// wallet. It also checks if fiat rates are expired and does some clean-up.
+// wallet.
 func (c *Core) fiatConversions() map[uint32]float64 {
-	supportedAssets := c.SupportedAssets()
+	supportedAssets := asset.Assets()
 
 	c.ratesMtx.RLock()
 	defer c.ratesMtx.RUnlock()
-	disabledCount := 0
 	fiatRatesMap := make(map[uint32]float64, len(supportedAssets))
 	for assetID := range supportedAssets {
 		var rateSum float64
-		var sources int32
-		for token, source := range c.fiatRateSources {
-			// Remove fiat rate source with expired exchange rate data.
-			if source.isExpired(fiatRateDataExpiry) {
-				delete(c.fiatRateSources, token)
-				disabledCount++
-				continue
-			}
+		var sources int
+		for _, source := range c.fiatRateSources {
 			rateInfo := source.assetRate(assetID)
 			if rateInfo != nil && time.Since(rateInfo.lastUpdate) < fiatRateDataExpiry {
 				sources++
@@ -7884,12 +7856,6 @@ func (c *Core) fiatConversions() map[uint32]float64 {
 			fiatRatesMap[assetID] = rateSum / float64(sources) // get average rate.
 		}
 	}
-
-	// Ensure disabled fiat rate fetchers are saved to database.
-	if disabledCount > 0 {
-		c.saveDisabledRateSources()
-	}
-
 	return fiatRatesMap
 }
 
@@ -7904,32 +7870,33 @@ func (c *Core) ToggleRateSourceStatus(source string, disable bool) error {
 
 // enableRateSource enables a fiat rate source.
 func (c *Core) enableRateSource(source string) error {
-	c.ratesMtx.Lock()
-	defer c.ratesMtx.Unlock()
-
 	// Check if it's an invalid rate source or it is already enabled.
 	rateFetcher, found := fiatRateFetchers[source]
 	if !found {
 		return errors.New("cannot enable unknown fiat rate source")
-	} else if c.fiatRateSources[source] != nil {
+	}
+
+	c.ratesMtx.Lock()
+	defer c.ratesMtx.Unlock()
+	if c.fiatRateSources[source] != nil {
 		return nil // already enabled.
 	}
 
 	// Build fiat rate source.
-	c.fiatRateSources[source] = newCommonRateSource(rateFetcher)
+	rateSource := newCommonRateSource(rateFetcher)
+	c.fiatRateSources[source] = rateSource
 
 	// If this is our first fiat rate source, start fiat rate fetcher goroutine,
 	// else fetch rates.
 	if len(c.fiatRateSources) == 1 {
 		c.fetchFiatExchangeRates()
 	} else {
-		source := c.fiatRateSources[source]
-		supportedAssets := c.SupportedAssets()
-		go func(source *commonRateSource) {
+		go func() {
+			supportedAssets := c.SupportedAssets() // not with ratesMtx locked!
 			ctx, cancel := context.WithTimeout(c.ctx, 4*time.Second)
 			defer cancel()
-			source.refreshRates(ctx, c.log, supportedAssets)
-		}(source)
+			rateSource.refreshRates(ctx, c.log, supportedAssets)
+		}()
 	}
 
 	// Update disabled fiat rate source.
@@ -7941,15 +7908,17 @@ func (c *Core) enableRateSource(source string) error {
 
 // disableRateSource disables a fiat rate source.
 func (c *Core) disableRateSource(source string) error {
-	c.ratesMtx.Lock()
-	defer c.ratesMtx.Unlock()
-
 	// Check if it's an invalid fiat rate source or it is already
 	// disabled.
 	_, found := fiatRateFetchers[source]
 	if !found {
 		return errors.New("cannot disable unknown fiat rate source")
-	} else if c.fiatRateSources[source] == nil {
+	}
+
+	c.ratesMtx.Lock()
+	defer c.ratesMtx.Unlock()
+
+	if c.fiatRateSources[source] == nil {
 		return nil // already disabled.
 	}
 
@@ -7963,6 +7932,27 @@ func (c *Core) disableRateSource(source string) error {
 	return nil
 }
 
+// removeExpiredRateSources disables expired fiat rate source.
+func (c *Core) removeExpiredRateSources() {
+	c.ratesMtx.Lock()
+	defer c.ratesMtx.Unlock()
+
+	// Remove fiat rate source with expired exchange rate data.
+	var disabledSources []string
+	for token, source := range c.fiatRateSources {
+		if source.isExpired(fiatRateDataExpiry) {
+			delete(c.fiatRateSources, token)
+			disabledSources = append(disabledSources, token)
+		}
+	}
+
+	// Ensure disabled fiat rate fetchers are saved to database.
+	if len(disabledSources) > 0 {
+		c.saveDisabledRateSources()
+		c.log.Warnf("Expired rate source(s) has been disabled: %v", strings.Join(disabledSources, ", "))
+	}
+}
+
 // saveDisabledRateSources saves disabled fiat rate sources to database and
 // shuts down rate fetching if there are no exchange rate source. Use under
 // ratesMtx lock.
@@ -7978,6 +7968,7 @@ func (c *Core) saveDisabledRateSources() {
 	if len(c.fiatRateSources) == 0 && c.stopFiatRateFetching != nil {
 		c.stopFiatRateFetching()
 		c.stopFiatRateFetching = nil
+		c.log.Debug("shutting down rate fetching")
 	}
 
 	err := c.db.SaveDisabledRateSources(disabled)

client/core/core_test.go

@@ -1089,10 +1089,10 @@ func randomMsgMarket() (baseAsset, quoteAsset *msgjson.Asset) {
 }
 
 func tFetcher(_ context.Context, log dex.Logger, _ map[uint32]*SupportedAsset) map[uint32]float64 {
-	fiatRates := make(map[uint32]float64)
-	fiatRates[tUTXOAssetA.ID] = 45
-	fiatRates[tUTXOAssetB.ID] = 32000
-	return fiatRates
+	return map[uint32]float64{
+		tUTXOAssetA.ID: 45,
+		tUTXOAssetB.ID: 32000,
+	}
 }
 
 type testRig struct {

client/core/exchangeratefetcher.go

@@ -24,7 +24,8 @@ const (
 	// fiatRateDataExpiry : Any data older than fiatRateDataExpiry will be discarded.
 	fiatRateDataExpiry = 60 * time.Minute
 
-	// Tokens. Used to identify fiat rate source.
+	// Tokens. Used to identify fiat rate source, source name must not contain a
+	// comma.
 	messari       = "Messari"
 	coinpaprika   = "Coinpaprika"
 	dcrdataDotOrg = "dcrdata"
@@ -56,26 +57,28 @@ type fiatRateInfo struct {
 type rateFetcher func(context context.Context, logger dex.Logger, assets map[uint32]*SupportedAsset) map[uint32]float64
 
 type commonRateSource struct {
-	lastRequest time.Time
-	fetchRates  rateFetcher
+	fetchRates rateFetcher
 
 	mtx       sync.RWMutex
 	fiatRates map[uint32]*fiatRateInfo
 }
 
 // isExpired checks the last update time for all fiat rates against the
-// provided expiryTime.
+// provided expiryTime. This only returns true if all rates are expired.
 func (source *commonRateSource) isExpired(expiryTime time.Duration) bool {
+	now := time.Now()
+
 	source.mtx.RLock()
 	defer source.mtx.RUnlock()
-	var expiredCount int
+	if len(source.fiatRates) == 0 {
+		return false
+	}
 	for _, rateInfo := range source.fiatRates {
-		if time.Since(rateInfo.lastUpdate) > expiryTime {
-			expiredCount++
+		if now.Sub(rateInfo.lastUpdate) < expiryTime {
+			return false // one not expired is enough
 		}
 	}
-	totalFiatRate := len(source.fiatRates)
-	return expiredCount == totalFiatRate && totalFiatRate != 0
+	return true
 }
 
 // assetRate returns the fiat rate information for the assetID specified. The
@@ -89,12 +92,13 @@ func (source *commonRateSource) assetRate(assetID uint32) *fiatRateInfo {
 // refreshRates updates the last update time and the rate information for assets.
 func (source *commonRateSource) refreshRates(ctx context.Context, logger dex.Logger, assets map[uint32]*SupportedAsset) {
 	fiatRates := source.fetchRates(ctx, logger, assets)
+	now := time.Now()
 	source.mtx.Lock()
 	defer source.mtx.Unlock()
 	for assetID, fiatRate := range fiatRates {
 		source.fiatRates[assetID] = &fiatRateInfo{
 			rate:       fiatRate,
-			lastUpdate: time.Now(),
+			lastUpdate: now,
 		}
 	}
 }

client/core/notification.go

@@ -356,16 +356,16 @@ func newConnEventNote(topic Topic, subject, host string, status comms.Connection
 	}
 }
 
-// fiatRatesNote is an update of fiat rate data for assets.
-type fiatRatesNote struct {
+// FiatRatesNote is an update of fiat rate data for assets.
+type FiatRatesNote struct {
 	db.Notification
 	FiatRates map[uint32]float64 `json:"fiatRates"`
 }
 
 const TopicFiatRatesUpdate Topic = "fiatrateupdate"
 
-func newFiatRatesUpdate(rates map[uint32]float64) *fiatRatesNote {
-	return &fiatRatesNote{
+func newFiatRatesUpdate(rates map[uint32]float64) *FiatRatesNote {
+	return &FiatRatesNote{
 		Notification: db.NewNotification(NoteTypeFiatRates, TopicFiatRatesUpdate, "", "", db.Data),
 		FiatRates:    rates,
 	}

client/db/interface.go

@@ -133,7 +133,7 @@ type DB interface {
 	// DisabledRateSources retrieves disabled fiat rate sources from the
 	// database.
 	DisabledRateSources() ([]string, error)
-	// SaveDisabledRateSources saves disabled fiat rate sources in the
-	// database.
+	// SaveDisabledRateSources saves disabled fiat rate sources in the database.
+	// A source name must not contain a comma.
 	SaveDisabledRateSources(disabledSources []string) error
 }

client/webserver/site/src/html/wallets.tmpl

@@ -76,7 +76,7 @@
             <div data-balance-target="{{.ID}}">
               {{.Info.UnitInfo.ConventionalString .Wallet.Balance.Available}}
             </div>
-             <div class="d-hide d-flex"><span data-conversion-target={{.ID}}></span>USD</span>
+            <div class="d-hide d-flex"><span data-conversion-target={{.ID}}></span>USD</span>
             </div>
             {{else}}
               0.00000000

client/webserver/site/src/js/app.ts

@@ -580,8 +580,8 @@ export default class Application {
         const bal = wallet.balance.available
         const balances = this.main.querySelectorAll(`[data-balance-target="${wallet.assetID}"]`)
         balances.forEach(el => { el.textContent = Doc.formatFullPrecision(bal, asset.info.unitinfo) })
-      }
         break
+      }
       case 'match': {
         const n = note as MatchNote
         const ord = this.order(n.orderID)

client/webserver/site/src/localized_html/en-US/wallets.tmpl

@@ -76,7 +76,7 @@
             <div data-balance-target="{{.ID}}">
               {{.Info.UnitInfo.ConventionalString .Wallet.Balance.Available}}
             </div>
-             <div class="d-hide d-flex"><span data-conversion-target={{.ID}}></span>USD</span>
+            <div class="d-hide d-flex"><span data-conversion-target={{.ID}}></span>USD</span>
             </div>
             {{else}}
               0.00000000